Review Request 123731: Cleanup handling of notifications closing

[Martin Klapetek]

> On May 13, 2015, 12:54 a.m., Jan Kundrát wrote:
> > Is it OK to have a non-null main object whose d-ptr is nullptr already? If not, can you fix the cause of that?

It's not and I don't fully understand why that happens (and as mentioned I could never reproduce), but it's what those backtraces suggest. I admit this is not ideal solution, but it at least should prevent crashes from happening.


> On May 13, 2015, 12:54 a.m., Jan Kundrát wrote:
> > src/notifybypopup.cpp, line 492
> > <https://git.reviewboard.kde.org/r/123731/diff/1/?file=368324#file368324line492>
> >
> >     Is there an enum for this? Using that instead of a comment which explains what a magic value is about sounds much better.

No; the reasons (the numbers) are however explained in the specification, dealing with this code does require knowledge of that spec. Also this is the only place actually using it and it uses only one value, I don't see much added value for a whole enum over a comment.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/123731/#review80265
-----------------------------------------------------------


On May 12, 2015, 1:59 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123731/
> -----------------------------------------------------------
> 
> (Updated May 12, 2015, 1:59 p.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: knotifications
> 
> 
> Description
> -------
> 
> While investigating the cause of crash of https://bugs.kde.org/show_bug.cgi?id=342752 I've come across some loose ends in how KNotifications is handling closing of notifications.
> 
> As for the crash itself, I never managed to reproduce even with special written test cases, but what happens (or seem to) is this:
>  * KNotification object gets deleted
>  * The destructor calls close() on all plugins
>  * The NotifyByPopup plugin does async dbus call to close the notification and returns
>  * KNotification's dpointer is deleted
>  * The DBus reply returns, calling NotifyByPopup::finished()
>  * Now for some reason the KNotification object is still not null but its dpointer is gone, so the check "if (!notification || d->notifications.contains(notification->id()))" crashes on the notification->id() call
> 
> So I've made it simply return -1 when dpointer is null, which should ideally prevent the crashes
> 
> 
> Diffs
> -----
> 
>   src/knotification.cpp 01962b3 
>   src/knotificationmanager.cpp 0d9b3b0 
>   src/knotificationmanager_p.h f8e7df8 
>   src/notifybypopup.cpp 2f84ab0 
> 
> Diff: https://git.reviewboard.kde.org/r/123731/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20150512/49e49737/attachment.html>