Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fix crash when stopping music - FRODO FOOD! #1971

Merged
merged 1 commit into from Dec 23, 2012

Conversation

Projects
None yet
5 participants
Owner

Memphiz commented Dec 21, 2012

When paplayer gets stopped in the last 5 secs where the next stream is already slaved the stop is blocked until the next track starts (no clue draining the stream i guess). In that case the onplaybackstarted callback for the next track is fired even if we are about to stop. In the message handler the current playlist is fetched and accessed directly with [] operator without checking if the playlist is still valid. This leads to bad access crashing because the playlist is empty already when the started message gets processed. This fix just fetches the playlist and checks the size before accessing the playlist item - fixes #13792

This is for frodo. I'm doing it as a PR because i was unsure if the early return true is right in that case. Talked about it with Montellese already and Martijn verified that this fixes the crash for windows too (and i did tests on osx).

@davilla @jmarshallnz for review

[fix] - when paplayer gets stopped in the last 5 secs where the next …
…stream is already slaved the stop is blocked until the next track starts (no clue draining the stream i guess). In that case the onplaybackstarted callback for the next track is fired even if we are about to stop. In the message handler the current playlist is fetched and accessed directly with [] operator without checking if the playlist is still valid. This leads to bad access crashing because the playlist is empty already when the started message gets processed. This fix just fetches the playlist and checks the size before accessing the playlist item - fixes #13792
Member

jmarshallnz commented Dec 21, 2012

Looks OK. I'd be tempted to reset m_nextPlaylistItem to -1 as well in both branches of that code (it'll normally be reset when you play something next, or the next file gets queued, but it should really be reset as soon as we've used it).

Owner

Memphiz commented Dec 21, 2012

Well your call (or davillas maybe) cause i don't know what else of behaviour that would change. (i would say "don't listen to the voices" for frodo if you are not 100 percent sure that it won't add regressions <- now thats impossible i guess haha).

Member

jmarshallnz commented Dec 21, 2012

Well, there's 2 choices here:

  1. early return (i.e. ignore the GUI_MSG_PLAYBACK_STARTED event).
  2. ignore the change of m_currentFile or whatever it is by moving the check into the first if() block.

I suspect the 2nd is less likely to result in regressions, but I'm not sure exactly what the expected behaviour is here (the playlist has been cleared, but the message is being fired even though the player is stopped?) Does GUI_MSG_PLAYBACK_STOPPED get sent straight after, or is this is a rogue event. If so, perhaps the rogue event should be eliminated in paplayer ?

Owner

Memphiz commented Dec 21, 2012

Its some sort of race. Start message is fired - player is stopped - start message is processed...

Owner

Memphiz commented Dec 21, 2012

I had 2. implemented before but was unsure if it was ok to do the rest of the message handler so i added the return true for that special case. Both approaches fixed the crash of course

Contributor

davilla commented Dec 22, 2012

I'm ok with either

Member

arnova commented Dec 22, 2012

This well could be the cause of issue I reported here: http://forum.xbmc.org/showthread.php?tid=141224&page=22

Contributor

DDDamian commented Dec 22, 2012

There is a potential deadlock in master as well where a draining stream is fading in PAPlayer::SoftStop and it can hang when PAPlayer exits - may be related.

Owner

Memphiz commented Dec 23, 2012

I decided to pull this in as is (because imo this start message is invalid and should just do nothing).

Memphiz added a commit that referenced this pull request Dec 23, 2012

Merge pull request #1971 from Memphiz/master
Fix crash when stopping music - FRODO FOOD!

@Memphiz Memphiz merged commit 5b01356 into xbmc:master Dec 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment