As outlined here: https://bugzilla.mozilla.org/show_bug.cgi?id=1648024#c1, we follow the MPRIS spec and use the PID, which is namespaced in a flatpak context, so we need a different form of differentiation there.

Could we use profile name for that as we use for the remote?

3 years ago https://bugzilla.mozilla.org/show_bug.cgi?id=1648024#c1 I suggested:

In a Flatpak context a PID is not a suitable key to unique-ify between different instances of Firefox. If you start a second Firefox with a different profile, it will be in a separate PID namespace, so both are likely to believe themselves to be PID 3. A better choice (which is also suitable in the non-Flatpak case) is to use something derived from the D-Bus connection's unique name.

These are defined in https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names-bus. The specification is a little wooly on the details but in practice they have the form ":I.J" for two integers I and J; adjusting for characters which are legal in D-Bus names, perhaps org.mpris.MediaPlayer2.firefox.instance_I_J would be a good option.

The severity field is not set for this bug.
:gerard-majax, could you have a look please?

For more information, please visit BugBot documentation.

(In reply to Will Thompson from comment #2)

3 years ago https://bugzilla.mozilla.org/show_bug.cgi?id=1648024#c1 I suggested:

In a Flatpak context a PID is not a suitable key to unique-ify between different instances of Firefox. If you start a second Firefox with a different profile, it will be in a separate PID namespace, so both are likely to believe themselves to be PID 3. A better choice (which is also suitable in the non-Flatpak case) is to use something derived from the D-Bus connection's unique name.

These are defined in https://dbus.freedesktop.org/doc/dbus-specification.html#message-protocol-names-bus. The specification is a little wooly on the details but in practice they have the form ":I.J" for two integers I and J; adjusting for characters which are legal in D-Bus names, perhaps org.mpris.MediaPlayer2.firefox.instance_I_J would be a good option.

As much as I understand, to get the unique name we need https://docs.gtk.org/gio/method.DBusConnection.get_unique_name.html which depends on a GDbusConnection that we dont have when we get into Open() https://searchfox.org/mozilla-central/rev/ffdc4971dc18e1141cb2a90c2b0b776365650270/widget/gtk/MPRISServiceHandler.cpp#300-327 where we call the g_bus_own_name().

You can get it with g_bus_get.

(In reply to Will Thompson from comment #6)

You can get it with g_bus_get.

You're right, I got something quite easily with g_bus_get_sync, I'm not sure how bad it is for us to perform that dbus connection in a sync manner?

(In reply to :gerard-majax from comment #7)

(In reply to Will Thompson from comment #6)

You can get it with g_bus_get.

You're right, I got something quite easily with g_bus_get_sync, I'm not sure how bad it is for us to perform that dbus connection in a sync manner?

g_bus_get_sync() is not a good idea and we should avoid it if possible. Will try to look at it.

(In reply to Martin Stránský [:stransky] (ni? me) from comment #8)

(In reply to :gerard-majax from comment #7)

(In reply to Will Thompson from comment #6)

You can get it with g_bus_get.

You're right, I got something quite easily with g_bus_get_sync, I'm not sure how bad it is for us to perform that dbus connection in a sync manner?

g_bus_get_sync() is not a good idea and we should avoid it if possible. Will try to look at it.

I already have something, g_bus_get() would require us to have a callback and get back the MPRISServiceHandler as user data, I can look into that later but I'm not sure I will have much time, so if you have more, feel free to steal my patch

Maybe it's good enough this way? Can both of you have a look at my patch ?

Backed out for GTest failure on MPRISServiceHandler.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/647a735896f8ff62ef385a0ebb9fd9a8bd8478d6
Log link: https://treeherder.mozilla.org/logviewer?job_id=433937643&repo=autoland&lineNumber=37160

It's due to assertion from async handler:

Assertion failure: !mInitialized, at /builds/worker/checkouts/gecko/widget/gtk/MPRISServiceHandler.cpp:339
#01: mozilla::widget::MPRISServiceHandler::OwnName(_GDBusConnection*) [widget/gtk/MPRISServiceHandler.cpp:339]
#02: mozilla::widget::g_bus_get_callback(_GObject*, _GAsyncResult*, void*) [widget/gtk/MPRISServiceHandler.cpp:0]
#03: ??? [/usr/lib/x86_64-linux-gnu/libgio-2.0.so.0 + 0x8d283]

(In reply to Martin Stránský [:stransky] (ni? me) from comment #14)

It's due to assertion from async handler:

Assertion failure: !mInitialized, at /builds/worker/checkouts/gecko/widget/gtk/MPRISServiceHandler.cpp:339
#01: mozilla::widget::MPRISServiceHandler::OwnName(_GDBusConnection*) [widget/gtk/MPRISServiceHandler.cpp:339]
#02: mozilla::widget::g_bus_get_callback(_GObject*, _GAsyncResult*, void*) [widget/gtk/MPRISServiceHandler.cpp:0]
#03: ??? [/usr/lib/x86_64-linux-gnu/libgio-2.0.so.0 + 0x8d283]

which makes no sense, since we get mInitialized=true at the end of OwnName. Or we get called multiple times ?

No local repro ...

You need to run debug build (MOZ_ASSERT() is performed on debug) or change the assertions to something else (MOZ_RELEASE_ASSERT ?).
But I don't see that either. I wonder if MPRIS service is available on mozilla test suite at all? Maybe we need to locally disable it (how?) to get similar results?

Ahh I see. There's MPRISServiceHandler::IsOpened() call which returns mInitialized. So it looks uninited during async call.

So you need to return mInitialized = true call to MPRISServiceHandler::Open()

(In reply to Martin Stránský [:stransky] (ni? me) from comment #17)

You need to run debug build (MOZ_ASSERT() is performed on debug) or change the assertions to something else (MOZ_RELEASE_ASSERT ?).
But I don't see that either. I wonder if MPRIS service is available on mozilla test suite at all? Maybe we need to locally disable it (how?) to get similar results?

[==========] Running 2 tests from 1 test suite.
[----------] Global test environment set-up.
[----------] 2 tests from TestAudioCallbackDriver
[ RUN      ] TestAudioCallbackDriver.StartStop
[       OK ] TestAudioCallbackDriver.StartStop (200 ms)
[ RUN      ] TestAudioCallbackDriver.SlowStart
TestSlowStart with rate 1000
Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[Parent 588162, Main Thread] WARNING: Could not get the program name for a cubeb stream.: 'NS_SUCCEEDED(rv)', file /home/alex/codaz/Mozilla/gecko-cinnabar/dom/media/CubebUtils.cpp:403
TestSlowStart with rate 8000
TestSlowStart with rate 44100
[       OK ] TestAudioCallbackDriver.SlowStart (577 ms)
[----------] 2 tests from TestAudioCallbackDriver (778 ms total)

[----------] Global test environment tear-down
[==========] 2 tests from 1 test suite ran. (778 ms total)
[  PASSED  ] 2 tests.
[Parent 588162, Main Thread] WARNING: '!gBasePath', file /home/alex/codaz/Mozilla/gecko-cinnabar/dom/quota/ActorsParent.cpp:1577
[Parent 588162, Main Thread] WARNING: profile-do-change must precede profile-before-change-qm!: file /home/alex/codaz/Mozilla/gecko-cinnabar/dom/quota/ActorsParent.cpp:1578
[ERROR glean_core::database] Failed to record metric 'dirtybit' into glean_internal_info: Error { kind: Rkv(IoError(Os { code: 2, kind: NotFound, message: "Aucun fichier ou dossier de ce type" })) }
[ERROR glean_core] Can't persist ping lifetime data: Error { kind: Rkv(IoError(Os { code: 2, kind: NotFound, message: "Aucun fichier ou dossier de ce type" })) }
terminate called without an active exception
Redirecting call to abort() to mozalloc_abort

Hit MOZ_CRASH(Redirecting call to abort() to mozalloc_abort
) at /home/alex/codaz/Mozilla/gecko-cinnabar/memory/mozalloc/mozalloc_abort.cpp:35
#01: mozalloc_abort (/home/alex/codaz/Mozilla/gecko-cinnabar/memory/mozalloc/mozalloc_abort.cpp:35)
#02: mozilla::CheckedInt<unsigned long>::value() const (/home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/include/mozilla/CheckedInt.h:560)
#03: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xa4f06)
#04: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xb6e6c)
#05: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xb6ed7)
#06: ??? (/home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/bin/gtest/libxul.so + 0x5ea4a7d)
#07: mozilla::Runnable::Release() (/home/alex/codaz/Mozilla/gecko-cinnabar/xpcom/threads/nsThreadUtils.cpp:66)
#08: NS_DispatchBackgroundTask(already_AddRefed<nsIRunnable>, unsigned int) (/home/alex/codaz/Mozilla/gecko-cinnabar/xpcom/threads/nsThreadUtils.cpp:521)
#09: mozilla::MockCubeb::ThreadFunction() (/home/alex/codaz/Mozilla/gecko-cinnabar/dom/media/gtest/MockCubeb.cpp:704)
#10: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xe6333)
#11: set_alt_signal_stack_and_start(PthreadCreateParams*) (/home/alex/codaz/Mozilla/gecko-cinnabar/mozglue/interposers/pthread_create_interposer.cpp:81)
#12: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x97ada)
#13: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x12847c)
#14: ??? (???:???)

Program /home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/bin/firefox (pid = 588162) received signal 11.
Stack:
#01: WasmTrapHandler(int, siginfo_t*, void*) (/home/alex/codaz/Mozilla/gecko-cinnabar/js/src/wasm/WasmSignalHandlers.cpp:801)
#02: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x42910)
#03: mozalloc_abort (/home/alex/codaz/Mozilla/gecko-cinnabar/memory/mozalloc/mozalloc_abort.cpp:35)
#04: mozilla::CheckedInt<unsigned long>::value() const (/home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/include/mozilla/CheckedInt.h:560)
#05: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xa4f06)
#06: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xb6e6c)
#07: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xb6ed7)
#08: ??? (/home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/bin/gtest/libxul.so + 0x5ea4a7d)
#09: mozilla::Runnable::Release() (/home/alex/codaz/Mozilla/gecko-cinnabar/xpcom/threads/nsThreadUtils.cpp:66)
#10: NS_DispatchBackgroundTask(already_AddRefed<nsIRunnable>, unsigned int) (/home/alex/codaz/Mozilla/gecko-cinnabar/xpcom/threads/nsThreadUtils.cpp:521)
#11: mozilla::MockCubeb::ThreadFunction() (/home/alex/codaz/Mozilla/gecko-cinnabar/dom/media/gtest/MockCubeb.cpp:704)
#12: ??? (/lib/x86_64-linux-gnu/libstdc++.so.6 + 0xe6333)
#13: set_alt_signal_stack_and_start(PthreadCreateParams*) (/home/alex/codaz/Mozilla/gecko-cinnabar/mozglue/interposers/pthread_create_interposer.cpp:81)
#14: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x97ada)
#15: ??? (/lib/x86_64-linux-gnu/libc.so.6 + 0x12847c)
#16: ??? (???:???)
Sleeping for 300 seconds.
Type 'gdb /home/alex/codaz/Mozilla/gecko-cinnabar/obj-browser-dbg/dist/bin/firefox 588162' to attach your debugger to this thread.
^C

(In reply to Martin Stránský [:stransky] (ni? me) from comment #19)

So you need to return mInitialized = true call to MPRISServiceHandler::Open()

Which I guess is fine because we move from g_bus_own_name to async g_bus_get but under the hood, g_bus_own_name was already calling the async g_bus_get: https://gitlab.gnome.org/GNOME/glib/-/blob/a1664c3fa3cf3282fcede6a5348e8a43ae097d8d/gio/gdbusnameowning.c#L648-691 so we are not making any invasive change at that point

gtest-1proc are passing on try

https://hg.mozilla.org/mozilla-central/rev/3b454e1f816a