D15638: force-finish canberra notifications on close()
cfeck added subscribers: dfaure, cfeck. cfeck added a comment. @dfaure, worth a respin? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D15638 To: sitter, broulik, jtamate Cc: cfeck, dfaure, kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
D15638: force-finish canberra notifications on close()
This revision was automatically updated to reflect the committed changes. Closed by commit R289:d4f51fdc1d53: force-finish canberra notifications on close() (authored by sitter). REPOSITORY R289 KNotifications CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D15638?vs=42037&id=43212 REVISION DETAIL https://phabricator.kde.org/D15638 AFFECTED FILES src/notifybyaudio_canberra.cpp To: sitter, broulik, jtamate Cc: kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
D15638: force-finish canberra notifications on close()
broulik accepted this revision. REPOSITORY R289 KNotifications BRANCH master REVISION DETAIL https://phabricator.kde.org/D15638 To: sitter, broulik, jtamate Cc: kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
D15638: force-finish canberra notifications on close()
jtamate accepted this revision. jtamate added a comment. This revision is now accepted and ready to land. Forget what I just wrote. I've seen that I was missing 15 lines in between then in the phabricator diff view. :-( Looks good to me. REPOSITORY R289 KNotifications BRANCH master REVISION DETAIL https://phabricator.kde.org/D15638 To: sitter, broulik, jtamate Cc: kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
D15638: force-finish canberra notifications on close()
jtamate added a comment. What would happen if finishNotification is called twice, In line 161 and then in line 192? My guess is that finished signal will be called twice with the same notification, and therefore KNotificationManager::notifyPluginFinished will be called twice. I don't know what will happen the second time. Perhaps a return after line 161 is needed? REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D15638 To: sitter, broulik Cc: kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
D15638: force-finish canberra notifications on close()
sitter created this revision. sitter added a reviewer: broulik. Herald added a project: Frameworks. Herald added a subscriber: kde-frameworks-devel. sitter requested review of this revision. REVISION SUMMARY `KNotification::close()` causes the manager to close the plugin for the notification and after that KNotification will call deleteLater() on itself. In the canberra variant of NotifyByAudio we handled this by calling ca_context_cancel to abort playback of the audio. This ultimately would still cause a finishCallback once the playback actually cancelled. The callback does arrive in an undefined amount of loop cycles later though. Put together this allowed for timing issues where deleteLater would run before the finishCallback arrived, giving finishCallback the risk of accessing a KNotification object past its lifetime and segfaulting. To prevent this from happening we'll finishNotification in the plugin's close(). This drops the notification out of the mapping hashes and tells the manager that we are done. finishCallback now returns immediately if it cannot find a mapping for a notification (i.e. it was close()d already). CHANGELOG: Fixed a crash caused by bad lifetime management of canberra-based audio notification BUG: 398695 TEST PLAN added qdebugs. without patch close() and thus deleteLater() happens before finishCallback() but the callback still does its thing. with patch finishCallback is noop. REPOSITORY R289 KNotifications BRANCH master REVISION DETAIL https://phabricator.kde.org/D15638 AFFECTED FILES src/notifybyaudio_canberra.cpp To: sitter, broulik Cc: kde-frameworks-devel, jtamate, michaelh, ngraham, bruns
