After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 764754 - '-' in application id: unbreak bus activation and notifications
'-' in application id: unbreak bus activation and notifications
Status: RESOLVED FIXED
Product: glib
Classification: Platform
Component: gio
unspecified
Other All
: Normal normal
: ---
Assigned To: gtkdev
gtkdev
Depends on:
Blocks:
 
 
Reported: 2016-04-07 23:05 UTC by Bastien Nocera
Modified: 2016-04-25 10:35 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
NotificationDaemon: Unbreak notifications coming from certain apps (1.23 KB, patch)
2016-04-07 23:06 UTC, Bastien Nocera
none Details | Review
GDesktopAppInfo: support bus activation with '-' (2.73 KB, patch)
2016-04-12 15:13 UTC, Allison Karlitskaya (desrt)
committed Details | Review

Description Bastien Nocera 2016-04-07 23:05:48 UTC
Untested, might help https://github.com/Ippytraxx/gnome-twitch/issues/75
Comment 1 Bastien Nocera 2016-04-07 23:06:04 UTC
Created attachment 325567 [details] [review]
NotificationDaemon: Unbreak notifications coming from certain apps

If an application contained a "-" in its D-Bus name, gnome-shell would
try to activate the wrong D-Bus path, as GApplication transforms the '-'
in D-Bus names to '_' in the D-Bus path.

For example com.gnome-twitch.app became /com/gnome_twitch/app when
gnome-shell was trying to access /com/gnome-twitch/app

See application_path_from_appid() in glib/gio/gapplicationimpl-dbus.c
Comment 2 Jasper St. Pierre (not reading bugmail) 2016-04-07 23:10:30 UTC
We follow the specification at https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-latest.html#dbus

"The above interface must be implemented at an object path determined as follows: starting with the well-known D-Bus name of the application, change all dots to slashes and prefix a slash."

If this isn't correct, we should fix the specification.
Comment 3 Bastien Nocera 2016-04-07 23:12:38 UTC
(In reply to Jasper St. Pierre from comment #2)
> We follow the specification at
> https://specifications.freedesktop.org/desktop-entry-spec/desktop-entry-spec-
> latest.html#dbus
> 
> "The above interface must be implemented at an object path determined as
> follows: starting with the well-known D-Bus name of the application, change
> all dots to slashes and prefix a slash."
> 
> If this isn't correct, we should fix the specification.

One of this, or:
https://git.gnome.org/browse/glib/tree/gio/gapplicationimpl-dbus.c#n322
is doing it wrong.

Allison?
Comment 4 Allison Karlitskaya (desrt) 2016-04-12 14:25:10 UTC
Strictly, the application is at fault for trying to use notifications while having a '-' in its bus name.

Jasper reasonably cites the desktop entry spec as evidence that gnome-shell is doing the right thing here, but this is not entirely appropriate.  The desktop entry spec is only really interested in describing the org.freedesktop.Application interface for D-Bus activation of apps.

As it is, any app with '-' in its bus name is not a candidate for D-Bus activation.  The desktop entry spec is somewhat sloppy with its reference to "reverse DNS convention" naming, but it's clear from inspecting code in GDesktopAppInfo (and making reasonable inferences from the instructions in the spec for obtaining the object path) that an app with '-' in its bus name will simply not work.  Indeed, GDesktopAppInfo is using g_dbus_is_interface_name() which will reject any such name.

So much for the desktop entry spec.  On to the notification spec: it says that in order to use notifications, the app must be D-Bus activatable.  Okay.  So the app doesn't (indeed, can't) support activation, but it wants to use notifications: this won't work.

The app needs to be fixed.

So although the spec points the blame at the app here, in practice, this is our (my) fault because of the mess that exists here.  GApplication's transformation of bus names into paths predates the standardisation of DBus activation of applications as part of the desktop entry specification, which has a lot to do with the inconsistency here.  We apparently failed to consider that at the time.

So what do we do now?  We could try to support new-style notifications for apps that are using bus names that are incapable of activation (by fixing gnome-shell and specifying this in the notification spec) or we could try to amend the desktop entry spec to specify the escaping process as GApplication performs it (which would also involve updating GDesktopAppInfo, which follows the letter of the spec fairly closely).

Another alternative is that we can issue warnings to people who try to use '-' in their application IDs, and fall back to the old freedesktop-style notifications in this case in order to avoid the problem.  A strict reading of the specifications suggests this to be the appropriate course of action.  New-style notifications can only be supported for apps that are D-Bus activatable, and an app with '-' in its name is not D-Bus activatable, so it's GIO's fault for trying to use new style notifications with such an app.

Considering the ugliness of all of this escaping and substitution, I actually sort of prefer this option.
Comment 5 Matthias Clasen 2016-04-12 14:30:30 UTC
I think a warning is a good first step, regardless of any further measure we might or might not take.
Comment 6 Allison Karlitskaya (desrt) 2016-04-12 14:59:17 UTC
It seems that we have a bunch of apps that have '-' in their desktop file names and DBusActivatable=yes that have been silently falling back to fork()/exec().

Out of a desire not to piss off the entire world once more, by forcing those people to rename their desktop files, I think we should just standardise the escaping from '-' to '_' and make '-' an officially valid part of app IDs.

Special mention goes to gnome-font-viewer, which installs a desktop file with a '-' in the name, specifies DBusActivatable=yes and installs a D-Bus service file correctly specifying --gapplication-service on the Exec= line, but doesn't actually accept this option.  The only reason this app currently works is that all of this is made dead by the fact that the app's name is rejected for having a '-' in it.  This will break when we start accepting '-', but it's already totally broken, and it's easy to fix.
Comment 7 Allison Karlitskaya (desrt) 2016-04-12 15:02:42 UTC
Review of attachment 325567 [details] [review]:

Considering my last comment, I believe this change to be appropriate (but I am not a gnome-shell reviewer).
Comment 8 Allison Karlitskaya (desrt) 2016-04-12 15:13:58 UTC
Created attachment 325810 [details] [review]
GDesktopAppInfo: support bus activation with '-'

GApplication has accepted any valid bus name as an application ID since
before the time of D-Bus activation.  This includes bus names with '-'.
Several applications have even attempted support bus activation with
these names, going as far as installing D-Bus service files, without
realising that they are silently falling back to fork()/exec() on
account of the name containing a dash.

The reason for the problem is that D-Bus object paths cannot contain
dashes.  We solved this problem privately in an unspecified way inside
of GApplication but substituting '_' in this case, but never made this
part of the Desktop Entry Specification.

The fact that these apps with '-' in the desktop file names aren't
actually using D-Bus activation is beside the point: their intent here
was clear.  Let's avoid forcing them to rename their desktop files again
by simply accepting '-' in desktop file names and munging the path in
the way that GApplication did so historically.

The new path escaping code here has been copied more or less verbatim
from GApplication's own code for the same purpose, with only the removal
of one irrelevant part.

An update to the desktop entry specification will follow.
Comment 9 Matthias Clasen 2016-04-12 15:24:01 UTC
Review of attachment 325810 [details] [review]:

Looks fine to me.
Comment 10 Jasper St. Pierre (not reading bugmail) 2016-04-12 16:49:59 UTC
Review of attachment 325567 [details] [review]:

I would want a comment with a link to this bug, or a patched version of the spec.
Comment 11 Emmanuele Bassi (:ebassi) 2016-04-12 17:17:24 UTC
As a side note, having dashes inside the app id breaks them under xdg-app as well; see bug 761298.
Comment 12 Matthias Clasen 2016-04-12 18:12:12 UTC
I don't think we're talking about accepting dashes in application ids or bus names here; the - is in the desktop file name, which will be transformed into the app id/bus name by replacing - with _.
Comment 13 Allison Karlitskaya (desrt) 2016-04-12 19:59:21 UTC
It's interesting that xdg-app has gotten caught up in the same problem.  In our case, since we have a backwards compatibility concern, I'm happy to allow the wider set to continue to function.  At the same time, I think it's okay for xdg-app (which is a completely new thing) to require something stricter.

My only concern is that this may cause problems for apps that want to containerise themselves without changing their bus name... but that's entirely for xdg-app to decide, imho.
Comment 14 Jasper St. Pierre (not reading bugmail) 2016-04-12 22:23:27 UTC
IMO, we should probably follow escaping rules like systemd has for automatically escaping "invalid characters" in app IDs and such.
Comment 15 Allison Karlitskaya (desrt) 2016-04-25 07:22:31 UTC
@Jasper:

I considered that as well, but ultimately I disagree, for two reasons.

First reason is that we already do it the way we do it, so let's not change that just for the sake of it.

The other reason is that systemd is interested in escaping a wide range of characters.  We have only to worry about the characters that are allowed in bus name components but not in bus path compoents.  That one character: '-'.

We also don't need to worry about round-tripping the conversion.  This is just to come up with a unique object path.  Some would have argued that we should just have used '/'.
Comment 16 Allison Karlitskaya (desrt) 2016-04-25 07:30:24 UTC
Comment on attachment 325810 [details] [review]
GDesktopAppInfo: support bus activation with '-'

Attachment 325810 [details] pushed as 3301b85 - GDesktopAppInfo: support bus activation with '-'
Comment 17 Allison Karlitskaya (desrt) 2016-04-25 10:35:23 UTC
See https://bugs.freedesktop.org/show_bug.cgi?id=95129 and https://bugzilla.gnome.org/show_bug.cgi?id=765518 for follow-ups.

I think this is more or less resolved now.