D11891: Fix crashes in NotifyByAudio when closing applications

This revision was not accepted when it landed; it landed in state "Needs Revision". This revision was automatically updated to reflect the committed changes. Closed by commit R289:59bfcffc43d2: Fix crashes in NotifyByAudio when closing applications (authored by aacid). Restricted Application adde

D11891: Fix crashes in NotifyByAudio when closing applications

cfeck added a comment. You could apply my spelling correction before committing :) REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D11891 To: aacid, #frameworks, cullmann, rjvbb, apol Cc: cfeck, rjvbb, mpyne, michaelh, ngraham, bruns

D11891: Fix crashes in NotifyByAudio when closing applications

apol accepted this revision. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D11891 To: aacid, #frameworks, cullmann, rjvbb, apol Cc: cfeck, rjvbb, mpyne, michaelh, ngraham, bruns

D11891: Fix crashes in NotifyByAudio when closing applications

aacid added a comment. Since i think i have genuinely addressed all of Rene's comments, i will commit this next week unless someone disagrees. REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D11891 To: aacid, #frameworks, cullmann, rjvbb Cc: cfeck, rjvbb, mpyn

D11891: Fix crashes in NotifyByAudio when closing applications

aacid added a comment. In D11891#239437 , @rjvbb wrote: > This is about better and more concise English. The queued connection is the indirect explanation why the patch is necessary, and thus comes after the direct explanation (the fact that the

D11891: Fix crashes in NotifyByAudio when closing applications

rjvbb added a comment. This is about better and more concise English. The queued connection is the indirect explanation why the patch is necessary, and thus comes after the direct explanation (the fact that there may be pending signals). Think of it as a courtesy to people who want to get to

D11891: Fix crashes in NotifyByAudio when closing applications

aacid added inline comments. INLINE COMMENTS > rjvbb wrote in notifybyaudio.cpp:148-150 > Slightly better English: > > // since stopping a mediaobject means it wont't emit finished(). However, > // we can receive pending finished() signals that were already emitted after > // playback comp

D11891: Fix crashes in NotifyByAudio when closing applications

aacid added a comment. In D11891#238999 , @rjvbb wrote: > So the difference here is that `finishNotification` isn't called if `notification == nullptr`, with the crucial difference probably being the fact that `m` isn't added multiple times to t

D11891: Fix crashes in NotifyByAudio when closing applications

rjvbb added a comment. (Oops, missed that one :-/) REPOSITORY R289 KNotifications REVISION DETAIL https://phabricator.kde.org/D11891 To: aacid, #frameworks, cullmann, rjvbb Cc: cfeck, rjvbb, mpyne, michaelh, ngraham

D11891: Fix crashes in NotifyByAudio when closing applications

cfeck added inline comments. INLINE COMMENTS > rjvbb wrote in notifybyaudio.cpp:148-150 > Slightly better English: > > // since stopping a mediaobject means it wont't emit finished(). However, > // we can receive pending finished() signals that were already emitted after > // playback comp

D11891: Fix crashes in NotifyByAudio when closing applications

rjvbb requested changes to this revision. rjvbb added a comment. This revision now requires changes to proceed. (sorry, keep forgetting to set the action. Consider this a change request at least for the typo in the comment ;)) REPOSITORY R289 KNotifications REVISION DETAIL https://phabri

D11891: Fix crashes in NotifyByAudio when closing applications

rjvbb added a comment. So the difference here is that `finishNotification` isn't called if `notification == nullptr`, with the crucial difference probably being the fact that `m` isn't added multiple times to the list of reusable items? Why isn't that logic part of `finishNotification()`

D11891: Fix crashes in NotifyByAudio when closing applications

aacid created this revision. aacid added reviewers: Frameworks, cullmann. Restricted Application added a project: Frameworks. aacid requested review of this revision. REVISION SUMMARY We have a race between close() and onAudioFinished() that resulted in the same Phonon::MediaObject being added