force-finish canberra notifications on close()
ClosedPublic

Authored by sitter on Sep 21 2018, 8:42 AM.

Details

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.

Diff Detail

Repository
R289 KNotifications
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
sitter created this revision.Sep 21 2018, 8:42 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptSep 21 2018, 8:42 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
sitter requested review of this revision.Sep 21 2018, 8:42 AM

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?

jtamate accepted this revision.Sep 21 2018, 9:47 AM

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.

This revision is now accepted and ready to land.Sep 21 2018, 9:47 AM
broulik accepted this revision.Oct 9 2018, 11:14 AM
This revision was automatically updated to reflect the committed changes.

@dfaure, worth a respin?