Closed Bug 1648874 Opened 4 years ago Closed 4 years ago

MPRISServiceHandler should call g_dbus_method_invocation_return_value

Categories

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

Unspecified
Linux
defect

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox79 --- wontfix
firefox82 --- fixed

People

(Reporter: ryan.hendrickson, Assigned: tony, NeedInfo)

References

Details

Attachments

(1 file)

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.

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.

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.

Priority: -- → P3

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.

Assignee: nobody → 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.

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 :)

Flags: needinfo?(tony)

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?

Pushed by rmaries@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4bc28a04b7cf
Send DBus reply to MPRIS commands r=chunmin
Status: UNCONFIRMED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 82 Branch
You need to log in before you can comment on or make changes to this bug.