D11891: Fix crashes in NotifyByAudio when closing applications

[Albert Astals Cid]

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 added a subscriber: kde-frameworks-devel.

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D11891?vs=31176=34156#toc

REPOSITORY
  R289 KNotifications

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D11891?vs=31176=34156

REVISION DETAIL
  https://phabricator.kde.org/D11891

AFFECTED FILES
  src/notifybyaudio.cpp

To: aacid, #frameworks, cullmann, rjvbb, apol
Cc: kde-frameworks-devel, cfeck, rjvbb, mpyne, michaelh, ngraham, bruns

D11891: Fix crashes in NotifyByAudio when closing applications

[Christoph Feck]

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

[Aleix Pol Gonzalez]

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

[Albert Astals Cid]

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, mpyne, michaelh, ngraham, bruns

D11891: Fix crashes in NotifyByAudio when closing applications

[Albert Astals Cid]

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 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.

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

[René J . V . Bertin]

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 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.

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

[Albert Astals Cid]

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 completed: phonon uses a queued connection for these signals.

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.

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

[Albert Astals Cid]

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 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.

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

[René J . V . Bertin]

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

[Christoph Feck]

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 completed: phonon uses a queued connection for these signals.

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

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

[René J . V . Bertin]

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://phabricator.kde.org/D11891

To: aacid, #frameworks, cullmann, rjvbb
Cc: rjvbb, mpyne, michaelh, ngraham

D11891: Fix crashes in NotifyByAudio when closing applications

[René J . V . Bertin]

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()`? 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?

INLINE COMMENTS

> notifybyaudio.cpp:148-150
> +// since stoping a mediaobject means it wont't emit finished() *BUT*
> +// since the finished signal is a queued connection in phonon it can 
> happen
> +// that the playing had already finished and we just had not got the 
> signal yet

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.

REPOSITORY
  R289 KNotifications

REVISION DETAIL
  https://phabricator.kde.org/D11891

To: aacid, #frameworks, cullmann
Cc: rjvbb, mpyne, michaelh, ngraham

D11891: Fix crashes in NotifyByAudio when closing applications

[Albert Astals Cid]

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 twice to m_reusablePhonons, which would 
result in a crash
  
  BUGS: 380114

REPOSITORY
  R289 KNotifications

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D11891

AFFECTED FILES
  src/notifybyaudio.cpp

To: aacid, #frameworks, cullmann
Cc: michaelh, ngraham