Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a crash in playerctld #215

Merged
merged 7 commits into from
Jan 17, 2021
Merged

Fix a crash in playerctld #215

merged 7 commits into from
Jan 17, 2021

Conversation

Fendse
Copy link
Contributor

@Fendse Fendse commented Jan 4, 2021

As part of the MPRIS standard, media players may optionally implement the interfaces org.freedesktop.MediaPlayer2.TrackList and org.freedesktop.MediaPlayer2.Playlists and send signals on those as well as sending PropertiesChanged signals for these interfaces' properties. While many players don't do this, some do. playerctld notices these messages but fails to understand them, causing it to crash every time a player sent such a message (such as when opening or closing any file in VLC)

These commits prevent that crash, and lay some groundwork for possibly having playerctld support those interfaces further in the future.

Some players, including VLC, will try to message playerctld on
the following interfaces:
* org.mpris.MediaPlayer2.TrackLis
* org.mpris.MediaPlayer2.Playlists
These are standard-compliant, but playerctld did not know about them
and would crash on receiving such messages.
Notably, TrackList.Tracks doesn't get updated yet
* Add some comments
* Refactor struct Player so that it keeps track of whether a given
  player appears to support the optional interfaces
  org.mpris.MediaPlayer2.TrackList and org.mpris.MediaPlayer2.Playlists
* Attempt to fetch and cache properties from those optional interfaces
* Note intent to listen for signals on optional MPRIS interfaces, in
  particular the TrackList one as it doesn't send the new values in
  PropertiesChanged signals. No actual work done yet, though.
@acrisci
Copy link
Member

acrisci commented Jan 4, 2021

Very nice, this looks like a good contribution. I'll review this as soon as I have some time.

@acrisci
Copy link
Member

acrisci commented Jan 9, 2021

The test failure here is legitimate. I can reproduce it locally.

@acrisci
Copy link
Member

acrisci commented Jan 9, 2021

What player are you using to test this?

@acrisci
Copy link
Member

acrisci commented Jan 9, 2021

In player_method_call_proxy_callback(), check if the interface they called is supported on the destination player.

More specifically, prevent playerctld from crashing when the only
running player disappears.
Yes, this was in the test suite, and no I somehow didn't notice it
anyway.
@Fendse
Copy link
Contributor Author

Fendse commented Jan 10, 2021

The test failure here is legitimate. I can reproduce it locally.

So could I, and I'm not sure why I didn't notice that myself in the first place.

The error was a null dereference causing playerctld to crash when the only open player is closed. I didn't notice it while dogfooding because I almost never close my web browser (which counts as a player).

But missing the fact that a test was yelling at me is embarrassing.

In player_method_call_proxy_callback(), check if the interface they called is supported on the destination player.

Good point, working on it.

What player are you using to test this?

I've mainly tested with VLC (version 3.0.11.1) so far, since that's the only player I use that tries to implement either of these two interfaces. Admittedly, that means I didn't test org.mpris.MediaPlayer2.Playlists interaction very thoroughly, since I couldn't find an implementation "in the wild" (VLC doesn't support it). VLC's support for org.mpris.MediaPlayer2.TrackList is a bit spotty as well, since there are signals that it should emit but seemingly never does (or at least they don't show up in bustle).

There should probably be proper tests as well, I'll see if I can add some.

@acrisci
Copy link
Member

acrisci commented Jan 17, 2021

I'm going to merge this as it is right now because it's good, but would appreciate some further work on those points.

@acrisci acrisci merged commit 90fd8a4 into altdesktop:master Jan 17, 2021
sersorrel pushed a commit to sersorrel/nixpkgs that referenced this pull request Sep 25, 2021
(cherry picked from commit b01290e)

Reason: fixes a crash with TrackList and Playlists interfaces [1]

[1]: altdesktop/playerctl#215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants