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
1=== modified file 'include/ActionModel.h'
2--- include/ActionModel.h 2015-06-16 08:11:33 +0000
3+++ include/ActionModel.h 2015-10-13 14:11:48 +0000
4@@ -23,16 +23,16 @@
5
6 class ActionModel : public QStringListModel {
7 Q_OBJECT
8+ Q_ENUMS(ActionsRoles)
9
10 public:
11 ActionModel(QObject *parent=nullptr);
12 virtual ~ActionModel();
13
14- virtual int rowCount(const QModelIndex &index) const;
15- virtual QVariant data(const QModelIndex &index, int role) const;
16- virtual QHash<int, QByteArray> roleNames() const;
17+ virtual int rowCount(const QModelIndex &index) const override;
18+ virtual QVariant data(const QModelIndex &index, int role) const override;
19+ virtual QHash<int, QByteArray> roleNames() const override;
20
21- Q_ENUMS(ActionsRoles)
22 enum ActionsRoles {
23 RoleActionLabel = Qt::UserRole + 1,
24 RoleActionId = Qt::UserRole + 2
25
26=== modified file 'include/Notification.h'
27--- include/Notification.h 2015-06-23 09:16:23 +0000
28+++ include/Notification.h 2015-10-13 14:11:48 +0000
29@@ -125,4 +125,7 @@
30 QString filterText(const QString& text);
31 };
32
33+Q_DECLARE_METATYPE(Notification::Urgency)
34+Q_DECLARE_METATYPE(Notification::Type)
35+
36 #endif /* NOTIFICATION_HPP_ */
37
38=== modified file 'include/NotificationModel.h'
39--- include/NotificationModel.h 2015-06-23 09:18:43 +0000
40+++ include/NotificationModel.h 2015-10-13 14:11:48 +0000
41@@ -40,9 +40,9 @@
42 NotificationModel(QObject *parent=nullptr);
43 virtual ~NotificationModel();
44
45- virtual int rowCount(const QModelIndex &parent) const;
46- virtual QVariant data(const QModelIndex &index, int role) const;
47- virtual QHash<int, QByteArray> roleNames() const;
48+ virtual int rowCount(const QModelIndex &parent) const override;
49+ virtual QVariant data(const QModelIndex &index, int role) const override;
50+ virtual QHash<int, QByteArray> roleNames() const override;
51
52 void insertNotification(const QSharedPointer<Notification> &n);
53 QList<QSharedPointer<Notification>> getAllNotifications() const;
54
55=== modified file 'include/NotificationPlugin.h'
56--- include/NotificationPlugin.h 2013-09-03 12:41:34 +0000
57+++ include/NotificationPlugin.h 2015-10-13 14:11:48 +0000
58@@ -28,8 +28,8 @@
59 Q_PLUGIN_METADATA(IID "org.qt-project.Qt.QQmlExtensionInterface")
60
61 public:
62- void registerTypes(const char *uri);
63- void initializeEngine(QQmlEngine *engine, const char *uri);
64+ void registerTypes(const char *uri) override;
65+ void initializeEngine(QQmlEngine *engine, const char *uri) override;
66 };
67
68 #endif // NOTIFICATIONS_PLUGIN_H
69
70=== modified file 'src/Notification.cpp'
71--- src/Notification.cpp 2015-07-06 18:13:16 +0000
72+++ src/Notification.cpp 2015-10-13 14:11:48 +0000
73@@ -53,7 +53,7 @@
74 p->body = "default text";
75 p->server = nullptr;
76 p->value = -2;
77- p->actionsModel = new ActionModel();
78+ p->actionsModel = new ActionModel(this);
79 }
80
81 Notification::Notification(NotificationID id, int displayTime, const Urgency ur, const QString &text, Type type, NotificationServer *srv, QObject *parent) :
82@@ -65,12 +65,12 @@
83 p->server = srv;
84 p->value = -2;
85 p->displayTime = displayTime;
86- p->actionsModel = new ActionModel();
87+ p->actionsModel = new ActionModel(this);
88 }
89
90 Notification::Notification(NotificationID id, int displayTime, const Urgency ur, Type type, NotificationServer *srv, QObject *parent) :
91 Notification(id, displayTime, ur, QString(), type, srv, parent){
92- p->actionsModel = new ActionModel();
93+ p->actionsModel = new ActionModel(this);
94 }
95
96 Notification::~Notification() {
97
98=== modified file 'src/NotificationModel.cpp'
99--- src/NotificationModel.cpp 2015-06-23 09:18:43 +0000
100+++ src/NotificationModel.cpp 2015-10-13 14:11:48 +0000
101@@ -46,7 +46,8 @@
102 }
103
104 NotificationModel::NotificationModel(QObject *parent) : QAbstractListModel(parent), p(new NotificationModelPrivate) {
105- p->displayedNotifications.append(QSharedPointer<Notification>(new Notification(0, -1, Notification::Normal, QString(), Notification::PlaceHolder)));
106+ p->displayedNotifications.append(QSharedPointer<Notification>(new Notification(0, -1, Notification::Normal, QString(), Notification::PlaceHolder),
107+ &QObject::deleteLater));
108 connect(&(p->timer), SIGNAL(timeout()), this, SLOT(timeout()));
109 p->timer.setSingleShot(true);
110 }
111@@ -74,41 +75,41 @@
112 QVariant NotificationModel::data(const QModelIndex &index, int role) const {
113 //printf("Data %d.\n", index.row());
114 if (!index.isValid())
115- return QVariant();
116+ return QVariant();
117
118 switch(role) {
119 case ModelInterface::RoleType:
120- return QVariant(p->displayedNotifications[index.row()]->getType());
121+ return p->displayedNotifications[index.row()]->getType();
122
123 case ModelInterface::RoleUrgency:
124- return QVariant(p->displayedNotifications[index.row()]->getUrgency());
125+ return p->displayedNotifications[index.row()]->getUrgency();
126
127 case ModelInterface::RoleId:
128- return QVariant(p->displayedNotifications[index.row()]->getID());
129+ return p->displayedNotifications[index.row()]->getID();
130
131 case ModelInterface::RoleSummary:
132- return QVariant(p->displayedNotifications[index.row()]->getSummary());
133+ return p->displayedNotifications[index.row()]->getSummary();
134
135 case ModelInterface::RoleBody:
136- return QVariant(p->displayedNotifications[index.row()]->getBody());
137+ return p->displayedNotifications[index.row()]->getBody();
138
139 case ModelInterface::RoleValue:
140- return QVariant(p->displayedNotifications[index.row()]->getValue());
141+ return p->displayedNotifications[index.row()]->getValue();
142
143 case ModelInterface::RoleIcon:
144- return QVariant(p->displayedNotifications[index.row()]->getIcon());
145+ return p->displayedNotifications[index.row()]->getIcon();
146
147 case ModelInterface::RoleSecondaryIcon:
148- return QVariant(p->displayedNotifications[index.row()]->getSecondaryIcon());
149+ return p->displayedNotifications[index.row()]->getSecondaryIcon();
150
151 case ModelInterface::RoleActions:
152 return QVariant::fromValue(p->displayedNotifications[index.row()]->getActions());
153
154 case ModelInterface::RoleHints:
155- return QVariant(p->displayedNotifications[index.row()]->getHints());
156+ return p->displayedNotifications[index.row()]->getHints();
157
158 case ModelInterface::RoleNotification:
159- return QVariant(p->displayedNotifications[index.row()]);
160+ return QVariant::fromValue(p->displayedNotifications[index.row()]);
161
162 default:
163 return QVariant();
164@@ -269,37 +270,44 @@
165 }
166
167 void NotificationModel::removeNotification(const NotificationID id) {
168- for(int i=0; i<p->ephemeralQueue.size(); i++) {
169- if(p->ephemeralQueue[i]->getID() == id) {
170- p->ephemeralQueue.erase(p->ephemeralQueue.begin() + i);
171- Q_EMIT queueSizeChanged(queued());
172- return;
173- }
174- }
175-
176- for(int i=0; i<p->snapQueue.size(); i++) {
177- if(p->snapQueue[i]->getID() == id) {
178- p->snapQueue.erase(p->snapQueue.begin() + i);
179- Q_EMIT queueSizeChanged(queued());
180- return;
181- }
182- }
183-
184- for(int i=0; i<p->interactiveQueue.size(); i++) {
185- if(p->interactiveQueue[i]->getID() == id) {
186- p->interactiveQueue.erase(p->interactiveQueue.begin() + i);
187- Q_EMIT queueSizeChanged(queued());
188- return;
189- }
190- }
191-
192- for(int i=0; i<p->displayedNotifications.size(); i++) {
193+ for (int i = 0; i < p->displayedNotifications.size(); i++) {
194 if(p->displayedNotifications[i]->getID() == id) {
195 deleteFromVisible(i);
196 timeout(); // Simulate a timeout so visual state is updated.
197 return;
198 }
199 }
200+
201+ for (auto it = p->ephemeralQueue.begin(); it != p->ephemeralQueue.end(); ++it) {
202+ QSharedPointer<Notification> n = *it;
203+
204+ if (n && n->getID() == id) {
205+ p->ephemeralQueue.erase(it);
206+ Q_EMIT queueSizeChanged(queued());
207+ return;
208+ }
209+ }
210+
211+ for (auto it = p->snapQueue.begin(); it != p->snapQueue.end(); ++it) {
212+ QSharedPointer<Notification> n = *it;
213+
214+ if (n && n->getID() == id) {
215+ p->snapQueue.erase(it);
216+ Q_EMIT queueSizeChanged(queued());
217+ return;
218+ }
219+ }
220+
221+ for (auto it = p->interactiveQueue.begin(); it != p->interactiveQueue.end(); ++it) {
222+ QSharedPointer<Notification> n = *it;
223+
224+ if (n && n->getID() == id) {
225+ p->interactiveQueue.erase(it);
226+ Q_EMIT queueSizeChanged(queued());
227+ return;
228+ }
229+ }
230+
231 // The ID was not found in any queue. Should it be an error case or not?
232 }
233
234
235=== modified file 'src/NotificationServer.cpp'
236--- src/NotificationServer.cpp 2015-07-06 18:13:16 +0000
237+++ src/NotificationServer.cpp 2015-10-13 14:11:48 +0000
238@@ -144,7 +144,7 @@
239 expireTimeout = 5000;
240 }
241
242- QSharedPointer<Notification> n(new Notification(id, expireTimeout, urg, ntype, this));
243+ QSharedPointer<Notification> n(new Notification(id, expireTimeout, urg, ntype, this), &QObject::deleteLater);
244 connect(n.data(), SIGNAL(dataChanged(unsigned int)), this, SLOT(onDataChanged(unsigned int)));
245 connect(n.data(), SIGNAL(completed(unsigned int)), this, SLOT(onCompleted(unsigned int)));
246
247@@ -156,15 +156,15 @@
248 // Spec forbids zero as return value.
249 if(idCounter == 0) {
250 idCounter = 1;
251-}
252+ }
253 }
254
255 QString NotificationServer::messageSender() {
256 QString sender(LOCAL_OWNER);
257- if (calledFromDBus()) {
258- sender = message().service();
259- }
260- return sender;
261+ if (calledFromDBus()) {
262+ sender = message().service();
263+ }
264+ return sender;
265 }
266
267 bool NotificationServer::isCmdLine() {
268
269=== modified file 'test/CMakeLists.txt'
270--- test/CMakeLists.txt 2015-06-01 09:42:45 +0000
271+++ test/CMakeLists.txt 2015-10-13 14:11:48 +0000
272@@ -1,4 +1,3 @@
273-
274 function(add_notification_test NAME)
275 add_executable(${NAME} "${NAME}.cpp")
276
277
278=== modified file 'test/notificationtest.cpp'
279--- test/notificationtest.cpp 2014-10-09 08:26:38 +0000
280+++ test/notificationtest.cpp 2015-10-13 14:11:48 +0000
281@@ -1,5 +1,6 @@
282 #include "Notification.h"
283 #include "NotificationModel.h"
284+#include "NotificationServer.h"
285
286 #include <QtTest/QtTest>
287
288@@ -24,6 +25,7 @@
289 void testConfirmationValue();
290 void testTextFilter();
291 void testTextFilter_data();
292+ void testReverseClose();
293 };
294
295 void TestNotifications::testSimpleInsertion() {
296@@ -55,43 +57,50 @@
297 }
298
299 void TestNotifications::testOrder() {
300- const int timeout = 5000;
301- QSharedPointer<Notification> n1(new Notification(1, timeout, Notification::Low, "low", Notification::Ephemeral));
302- QSharedPointer<Notification> n2(new Notification(2, timeout, Notification::Normal, "high", Notification::Ephemeral));
303- QSharedPointer<Notification> n3(new Notification(3, timeout, Notification::Critical, "critical", Notification::Ephemeral));
304- NotificationModel m;
305-
306- m.insertNotification(n1);
307- QVERIFY(m.showingNotification(n1->getID()));
308-
309- m.insertNotification(n2);
310- QVERIFY(!m.showingNotification(n1->getID()));
311- QVERIFY(m.showingNotification(n2->getID()));
312- QVERIFY(m.queued() == 1);
313-
314- m.insertNotification(n3);
315- QVERIFY(!m.showingNotification(n1->getID()));
316- QVERIFY(!m.showingNotification(n2->getID()));
317- QVERIFY(m.showingNotification(n3->getID()));
318- QCOMPARE(m.queued(), 2);
319-
320- m.removeNotification(n3->getID());
321- QVERIFY(!m.showingNotification(n1->getID()));
322- QVERIFY(m.showingNotification(n2->getID()));
323- QVERIFY(!m.showingNotification(n3->getID()));
324- QCOMPARE(m.queued(), 1);
325-
326- m.removeNotification(n2->getID());
327- QVERIFY(m.showingNotification(n1->getID()));
328- QVERIFY(!m.showingNotification(n2->getID()));
329- QVERIFY(!m.showingNotification(n3->getID()));
330- QCOMPARE(m.queued(), 0);
331-
332- m.removeNotification(n1->getID());
333- QVERIFY(!m.showingNotification(n1->getID()));
334- QVERIFY(!m.showingNotification(n2->getID()));
335- QVERIFY(!m.showingNotification(n3->getID()));
336- QCOMPARE(m.queued(), 0);
337+ const int timeout = 1000;
338+ static NotificationModel *m = new NotificationModel(this);
339+ static NotificationServer *s = new NotificationServer(QDBusConnection::sessionBus(), *m);
340+ QStringList actions = QStringList();
341+ QVariantMap hints;
342+ int id[3];
343+
344+ hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Low);
345+ id[0] = s->Notify ("test-name-low", 0, "icon-low", "summary-low", "body-low", actions, hints, timeout);
346+ QVERIFY(m->showingNotification(id[0]));
347+
348+ hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Normal);
349+ id[1] = s->Notify ("test-name-normal", 0, "icon-normal", "summary-normal", "body-normal", actions, hints, timeout);
350+ QVERIFY(!m->showingNotification(id[0]));
351+ QVERIFY(m->showingNotification(id[1]));
352+ QVERIFY(m->queued() == 1);
353+
354+ hints[URGENCY_HINT] = QVariant::fromValue(Notification::Urgency::Critical);
355+ id[2] = s->Notify ("test-name-critical", 0, "icon-critical", "summary-critical", "body-critical", actions, hints, timeout);
356+ QVERIFY(!m->showingNotification(id[0]));
357+ QVERIFY(!m->showingNotification(id[1]));
358+ QVERIFY(m->showingNotification(id[2]));
359+ QCOMPARE(m->queued(), 2);
360+
361+ m->removeNotification(id[2]);
362+ QVERIFY(!m->showingNotification(id[0]));
363+ QVERIFY(m->showingNotification(id[1]));
364+ QVERIFY(!m->showingNotification(id[2]));
365+ QCOMPARE(m->queued(), 1);
366+
367+ m->removeNotification(id[1]);
368+ QVERIFY(m->showingNotification(id[0]));
369+ QVERIFY(!m->showingNotification(id[1]));
370+ QVERIFY(!m->showingNotification(id[2]));
371+ QCOMPARE(m->queued(), 0);
372+
373+ m->removeNotification(id[0]);
374+ QVERIFY(!m->showingNotification(id[0]));
375+ QVERIFY(!m->showingNotification(id[1]));
376+ QVERIFY(!m->showingNotification(id[2]));
377+ QCOMPARE(m->queued(), 0);
378+
379+ delete s;
380+ delete m;
381 }
382
383 void TestNotifications::testHas() {
384@@ -259,5 +268,38 @@
385 QCOMPARE(n.getBody(), result);
386 }
387
388+void TestNotifications::testReverseClose() {
389+ const int timeout = 1000;
390+ const int max = 20;
391+ static NotificationModel *m = new NotificationModel();
392+ static NotificationServer *s = new NotificationServer(QDBusConnection::sessionBus(), *m);
393+ QStringList actions = QStringList();
394+ QVariantMap hints;
395+ int before = m->numNotifications();
396+
397+ for (int i = 1; i <= max; i++) {
398+ s->Notify ("test-app-name",
399+ 0,
400+ "test-icon",
401+ "test-summary",
402+ "test-body",
403+ actions,
404+ hints,
405+ timeout);
406+ }
407+
408+ QCOMPARE(m->numNotifications(), max+1);
409+
410+ for (int i = max; i >= 1; i--) {
411+ m->getNotification(i)->close();
412+ QCOMPARE(m->numNotifications(), i);
413+ }
414+
415+ QCOMPARE(m->numNotifications(), before);
416+
417+ delete s;
418+ delete m;
419+}
420+
421 QTEST_MAIN(TestNotifications)
422 #include "notificationtest.moc"

Subscribers

People subscribed via source and target branches

to all changes: