1648874 - MPRISServiceHandler should call g_dbus_method_invocation_return_value

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect, P3)

Description

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

AFAIK this is only an issue for whatever client program is calling the MPRIS D-Bus methods (e.g., gsd-media-keys on GNOME), but I believe the correct thing to do when implementing a D-Bus method is to call g_dbus_method_invocation_return_value when the handler succeeds, even if there isn't a value to return. Otherwise, the client program will wait and eventually receive a timeout error.

So the code at https://dxr.mozilla.org/mozilla-central/rev/c68fe15a81fc2dc9fc5765f3be2573519c09b6c1/widget/gtk/MPRISServiceHandler.cpp#159 should have an else branch that does that.

Comment 1

[Ryan Hendrickson [rhendric on GitLab, GitHub]]

AFAIK this is only an issue for whatever client program

Actually strike that, I believe this is a memory leak in Firefox. Calling a g_dbus_method_invocation_return_* function is what frees the GDBusMethodInvocation instance.

Comment 2

[Yaroslav Isakov]

I've added g_dbus_method_invocation_return_value(aInvocation, NULL) to suggested place, and now playerctl (MPRIS2 client) is not hanging after issuing command to Firefox.

Comment 3

[tony]

Attachment: Bug 1648874 - Send DBus reply to MPRIS commands r?chunmin

Use g_dbus_method_invocation_return_value() to return a reply to DBus
clients that call service methods. Sending a reply is required by the
DBus specification and failing to do so may cause subtle bugs in
clients.

Comment 4

[tony]

Hi,

Thanks for improving media player support on the Linux desktop. There's a lot of interest in using this feature among Playerctl users. I've gotten 5-6 people in several reports on the repo who are affected by this bug. After examining dbus-monitor output, I've determined that the problem is that Firefox is not sending a method reply to these commands. The above patch fixes the issue by ensuring a reply is sent.

The issue can also be reproduced with a dbus-send command like the following:

dbus-send --session --print-reply --dest=org.mpris.MediaPlayer2.firefox.instance26047 /org/mpris/MediaPlayer2 org.mpris.MediaPlayer2.Player.Play

I would expect this command to exit quickly once the reply is received. Instead it hangs even though the command is completed successfully in Firefox.

Sending a reply to method invocations is required by the DBus specification.

https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-types-method

When an application handles a method call message, it is required to return a reply. The reply is identified by a REPLY_SERIAL header field indicating the serial number of the METHOD_CALL being replied to. The reply can have one of two types; either METHOD_RETURN or ERROR.

Let me know if there's anything else I need to do to get this issue resolved. This is my first contribution to Firefox.

Comment 5

[C.M.Chang[:chunmin]]

In case you don't know how to land the patch.

1. click the "Edit Revision"
2. select "Tags"
3. Enter "Check-in Needed"
   then the release sheriff would take the responsibility to land the patch

Thanks for contributing :)

Comment 6

[Cristian Brindusan [:cbrindusan]]

This patch failed to land due to the following error:
"Reason:
We're sorry, Lando could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg import --no-commit -s 95 /tmp/tmpqeq6unrq: applying /tmp/tmpqeq6unrq

patching file widget/gtk/MPRISServiceHandler.cpp
Hunk #2 FAILED at 808
1 out of 2 hunks FAILED -- saving rejects to file widget/gtk/MPRISServiceHandler.cpp.rej
abort: patch failed to apply"

Can you rebase your commits and try again?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4bc28a04b7cf
Send DBus reply to MPRIS commands r=chunmin

Comment 8

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4bc28a04b7cf