Fix crashes in NotifyByAudio when closing applications
ClosedPublic

Authored by aacid on Apr 2 2018, 9:10 PM.

Details

Summary

We have a race between close() and onAudioFinished() that resulted in the same
Phonon::MediaObject being added twice to m_reusablePhonons, which would result in a crash

BUGS: 380114

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.
aacid created this revision.Apr 2 2018, 9:10 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptApr 2 2018, 9:10 PM
aacid requested review of this revision.Apr 2 2018, 9:10 PM
mpyne added a subscriber: mpyne.Apr 3 2018, 3:03 AM
rjvbb added a subscriber: rjvbb.Apr 3 2018, 9:32 AM

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()? IOW, is there a valid reason for finishNotification to be called with a NULL notification argument but m that hasn't yet been moved from m_notifications to m_reusablePhotons?

src/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 completed: phonon uses a queued connection for these signals.
rjvbb requested changes to this revision.Apr 3 2018, 9:35 AM

(sorry, keep forgetting to set the action. Consider this a change request at least for the typo in the comment ;))

This revision now requires changes to proceed.Apr 3 2018, 9:35 AM
cfeck added a subscriber: cfeck.Apr 3 2018, 9:42 AM
cfeck added inline comments.
src/notifybyaudio.cpp
148–150

wont't -> won't (or will not)

rjvbb added a comment.Apr 3 2018, 10:21 AM

(Oops, missed that one :-/)

aacid added a comment.Apr 3 2018, 9:52 PM

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?

Yes, that's the commit log says

Why isn't that logic part of finishNotification()? IOW, is there a valid reason for finishNotification to be called with a NULL notification argument but m that hasn't yet been moved from m_notifications to m_reusablePhotons?

no, that should never happen.

aacid added inline comments.Apr 3 2018, 9:55 PM
src/notifybyaudio.cpp
148–150

I disagree it is a better explanation, i do understand my text better. the fact that it is a queued connection is important and needs to be at the beginning of the explanation and not at the end.

rjvbb added a comment.Apr 3 2018, 11:53 PM

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 the point first and maybe deal with the finer detail later.

The problem with this whole comment is that it's long and not very easy to follow for devs who are not (very) familiar with the code already (and those who are might not need all the detail). Rereading it with after-bedtime eyes I think you should probably just leave only the 1st sentence. The explanation why you can end up "here" after close was called could be put in the commit message, or as a "warning" above the connect() call that creates the connection.

Come to think of it, your patch could take the form below because there is already a check of notification:

if (Q_UNLIKELY(!notification)) {
    return;
} else if ((notification->flags() & KNotification::LoopSound)) {
    m->play();
    return;
}

Maybe merge your comment with the one about "if the sound is short enough" because I from what I understand that describes more or less the same situation.

aacid added a comment.Apr 4 2018, 11:08 PM

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 the point first and maybe deal with the finer detail later.

The problem with this whole comment is that it's long and not very easy to follow for devs who are not (very) familiar with the code already (and those who are might not need all the detail). Rereading it with after-bedtime eyes I think you should probably just leave only the 1st sentence. The explanation why you can end up "here" after close was called could be put in the commit message, or as a "warning" above the connect() call that creates the connection.

I disagree, i've read that code lots of times and it took me finding a semi-reproducible case to figure out what was wrong, so having a comment helps people that are familiar with it.

Come to think of it, your patch could take the form below because there is already a check of notification:

if (Q_UNLIKELY(!notification)) {
    return;
} else if ((notification->flags() & KNotification::LoopSound)) {
    m->play();
    return;
}

Maybe merge your comment with the one about "if the sound is short enough" because I from what I understand that describes more or less the same situation.

No, it describes a different situation, it's about

m->enqueue(soundURL);

not having had time to finish and thus onAudioSourceChanged not triggering.

aacid added a comment.May 7 2018, 10:08 PM

Since i think i have genuinely addressed all of Rene's comments, i will commit this next week unless someone disagrees.

apol accepted this revision.May 7 2018, 10:51 PM
cfeck added a comment.May 7 2018, 11:49 PM

You could apply my spelling correction before committing :)

This revision was not accepted when it landed; it landed in state Needs Revision.May 14 2018, 5:46 PM
This revision was automatically updated to reflect the committed changes.
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald TranscriptMay 14 2018, 5:46 PM