Re: Review Request 129092: Fix crashes in NotifyByAudio

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/
---

(Updated Oct. 5, 2016, 12:41 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Martin Klapetek.


Repository: knotifications


Description
---

Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
not called and thus finished() was not called and then when closing the app, 
the notification manager would go and delete all the non finished 
notifications, meaning it would call close() again, add the same 
Phonon::MediaObject to m_reusablePhonons again and then crash because it would 
delete them twice in the destructor


Diffs
-

  src/notifybyaudio.h 6726a93 
  src/notifybyaudio.cpp 903e996 

Diff: https://git.reviewboard.kde.org/r/129092/diff/


Testing
---

configure konsole notifications to emit sound on "Bell in Visible Session" and 
also show an info box.

Make it bell and quickly close the info box.

Close konsole.

Without this patch it will crash.


Thanks,

Albert Astals Cid

Re: Review Request 129092: Fix crashes in NotifyByAudio

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99788
---


Ship it!




- David Edmundson


On Oct. 4, 2016, 8:54 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Oct. 4, 2016, 8:54 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129092: Fix crashes in NotifyByAudio

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/
---

(Updated Oct. 4, 2016, 8:54 p.m.)


Review request for KDE Frameworks and Martin Klapetek.


Changes
---

Fix repeat condition as found by David


Repository: knotifications


Description
---

Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
not called and thus finished() was not called and then when closing the app, 
the notification manager would go and delete all the non finished 
notifications, meaning it would call close() again, add the same 
Phonon::MediaObject to m_reusablePhonons again and then crash because it would 
delete them twice in the destructor


Diffs (updated)
-

  src/notifybyaudio.h 6726a93 
  src/notifybyaudio.cpp 903e996 

Diff: https://git.reviewboard.kde.org/r/129092/diff/


Testing
---

configure konsole notifications to emit sound on "Bell in Visible Session" and 
also show an info box.

Make it bell and quickly close the info box.

Close konsole.

Without this patch it will crash.


Thanks,

Albert Astals Cid

Re: Review Request 129092: Fix crashes in NotifyByAudio

[Albert Astals Cid]

> On Oct. 4, 2016, 8:18 a.m., David Edmundson wrote:
> > Is it this? https://bugs.kde.org/show_bug.cgi?id=348414

Sadly I don't think so, i can only reproduce a crash at closing by doing what i 
say on Testing Done, it may fix it, but i can't prove it. I've looked at the 
code quite a lot and can't see how that bug would happen tbh.


> On Oct. 4, 2016, 8:18 a.m., David Edmundson wrote:
> > src/notifybyaudio.cpp, line 156
> > 
> >
> > We only want this sound looping check to be in ::onAudioFinished() not 
> > called from ::close()

right, will fix.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99754
---


On Oct. 3, 2016, 9:31 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Oct. 3, 2016, 9:31 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129092: Fix crashes in NotifyByAudio

[David Edmundson]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99754
---



Is it this? https://bugs.kde.org/show_bug.cgi?id=348414


src/notifybyaudio.cpp (line 155)


We only want this sound looping check to be in ::onAudioFinished() not 
called from ::close()


- David Edmundson


On Oct. 3, 2016, 9:31 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Oct. 3, 2016, 9:31 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129092: Fix crashes in NotifyByAudio

[Anthony Fieroni]

> On Oct. 4, 2016, 7:32 a.m., Anthony Fieroni wrote:
> > This is not a correct place to patch it. What is your used phonon backend?
> 
> Albert Astals Cid wrote:
> I disagree with you, none of the phonon backends call finished from stop, 
> because finished means "i reached the end of the file" while stop just means 
> stop.

Oh, you are right, i saw the api dox


- Anthony


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99749
---


On Oct. 4, 2016, 12:31 a.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Oct. 4, 2016, 12:31 a.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129092: Fix crashes in NotifyByAudio

[Albert Astals Cid]

> On Oct. 4, 2016, 4:32 a.m., Anthony Fieroni wrote:
> > This is not a correct place to patch it. What is your used phonon backend?

I disagree with you, none of the phonon backends call finished from stop, 
because finished means "i reached the end of the file" while stop just means 
stop.


> On Oct. 4, 2016, 4:32 a.m., Anthony Fieroni wrote:
> > src/notifybyaudio.cpp, line 145
> > 
> >
> > if (notification) {
> > finish...

No, read the code better.


- Albert


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99749
---


On Oct. 3, 2016, 9:31 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Oct. 3, 2016, 9:31 p.m.)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Re: Review Request 129092: Fix crashes in NotifyByAudio

[Anthony Fieroni]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/#review99749
---



This is not a correct place to patch it. What is your used phonon backend?


src/notifybyaudio.cpp (line 144)


if (notification) {
finish...


- Anthony Fieroni


On Окт. 4, 2016, 12:31 преди обяд, Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129092/
> ---
> 
> (Updated Окт. 4, 2016, 12:31 преди обяд)
> 
> 
> Review request for KDE Frameworks and Martin Klapetek.
> 
> 
> Repository: knotifications
> 
> 
> Description
> ---
> 
> Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
> not called and thus finished() was not called and then when closing the app, 
> the notification manager would go and delete all the non finished 
> notifications, meaning it would call close() again, add the same 
> Phonon::MediaObject to m_reusablePhonons again and then crash because it 
> would delete them twice in the destructor
> 
> 
> Diffs
> -
> 
>   src/notifybyaudio.h 6726a93 
>   src/notifybyaudio.cpp 903e996 
> 
> Diff: https://git.reviewboard.kde.org/r/129092/diff/
> 
> 
> Testing
> ---
> 
> configure konsole notifications to emit sound on "Bell in Visible Session" 
> and also show an info box.
> 
> Make it bell and quickly close the info box.
> 
> Close konsole.
> 
> Without this patch it will crash.
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

Review Request 129092: Fix crashes in NotifyByAudio

[Albert Astals Cid]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129092/
---

Review request for KDE Frameworks and Martin Klapetek.


Repository: knotifications


Description
---

Phonon::MediaObject::stop does not emit finished and thus onAudioFinished is 
not called and thus finished() was not called and then when closing the app, 
the notification manager would go and delete all the non finished 
notifications, meaning it would call close() again, add the same 
Phonon::MediaObject to m_reusablePhonons again and then crash because it would 
delete them twice in the destructor


Diffs
-

  src/notifybyaudio.h 6726a93 
  src/notifybyaudio.cpp 903e996 

Diff: https://git.reviewboard.kde.org/r/129092/diff/


Testing
---

configure konsole notifications to emit sound on "Bell in Visible Session" and 
also show an info box.

Make it bell and quickly close the info box.

Close konsole.

Without this patch it will crash.


Thanks,

Albert Astals Cid