GNOME Bugzilla – Bug 754983
Wayland: g_desktop_app_info_launch_uris_with_spawn() forces DISPLAY variable to Wayland display
Last modified: 2015-10-12 14:07:55 UTC
Created attachment 311259 [details] Simple reproducer with gtk+ alone, just build and run that on wayland and click on the "Website" link. When using gtk_show_uri() from a Wayland application (e.g. from the gtk about dialog), firefox (X11 only for now) will crash at startup. It appears that g_desktop_app_info_launch_uris_with_spawn() will sets/forces the value of DISPLAY to "wayland-0" which is not the X11 display that firefox expects, but the Wayland display instead. This is because g_desktop_app_info_launch_uris_with_spawn() does: 2717 display = g_app_launch_context_get_display (launch_context, 2718 G_APP_INFO (info), 2719 launched_files); 2720 if (display) 2721 envp = g_environ_setenv (envp, "DISPLAY", display, TRUE); 2722 But on Wayland, g_app_launch_context_get_display() will return the Wayland display name because the context maps to a GdkWaylandDisplay and not a GdkX11Display as expected. So either g_desktop_app_info_launch_uris_with_spawn() should not set the DISPLAY (but that would kinda defeat the whole idea of using a context from a given display/screen) or it should be aware of the backend used to decide which env variable to set for the display. Has this been considered before? Would that be an acceptable design?
Thanks for pointing this out, it hasn't come up before. The right fix would be to give the app launch context full control of the environment, with a method like: g_app_launch_context_get_environ() That would also get us out of dealing with DISPLAY in gio, which seems odd, layering-wise.
Right, if I understand correctly, g_app_launch_context_get_environ() would be a new API that would invoke a corresponding get_environ() method class dependent just like get_display() does, correct? There is already a g_app_launch_context_get_environment() method which returns g_get_environ() so this could be a bit confusing (g_app_launch_context_get_environment() vs. g_app_launch_context_get_environ()).
Oh interesting, I had forgotten about that. So, what currently happens in launch_uris is this: envp = g_app_launch_context_get_environment (ctx); ... display = g_app_launch_context_get_display (ctx); g_environ_setenv (envp, "DISPLAY=... So the app launch context could already set up DISPLAY in get_environment() anyway, just by calling g_app_launch_context_setenv (...). I would suggest that is what we should do, then: 1. Make _gdk_x11_display_get_app_launch_context call g_app_launch_context_setenv (ctx, DISPLAY...) 2. Drop the explicit DISPLAY handling in gio 3. Possibly doing the same for the Wayland implementation (but it shouldn't need WAYLAND_DISPLAY to be set, afaik)
Created attachment 311369 [details] [review] glib/gio patch glib patch: Do set the DISPLAY in gio
Created attachment 311370 [details] [review] gtk+/gdk/x11 patch gtk+/gdk/x11 patch: Set DISPLAY in _gdk_x11_display_get_app_launch_context()
Created attachment 311371 [details] [review] gtk+/gdk/wayland patch gtk+/gdk/wayland patch: Set WAYLAND_DISPLAY in _gdk_wayland_display_get_app_launch_context()
- Tested to fix the issue on Wayland, - Tested it still works on X11, :) - Checked that DISPLAY is set appropriately on X11 and WAYLAND_DISPLAY on Wayland I kept two different patches for the X11 and WAYLAND parts in case we'd want not to apply the patch for Wayland or even revert it later (WAYLAND_DISPLAY is not mandatory, but it's been made mandatory for short while iirc - in doubt, I think it doesn't hurt to set the WAYLAND_DISPLAY as this is what is expected when using an app context for a given display). CAVEAT: It may be problematic on gtk2 as gtk2 relies on gio to set the DISPLAY and doesn't have _gdk_x11_display_get_app_launch_context()
(In reply to Olivier Fourdan from comment #7) > [...] > CAVEAT: It may be problematic on gtk2 as gtk2 relies on gio to set the > DISPLAY and doesn't have _gdk_x11_display_get_app_launch_context() I also check it doesn't break gtk2, at least from an X11 session, the DISPLAY is already set so it still works (which is most likely the most common use of this API).
Review of attachment 311369 [details] [review]: ::: gio/gdesktopappinfo.c @@ +2717,3 @@ display = g_app_launch_context_get_display (launch_context, G_APP_INFO (info), launched_files); No need to call g_app_launch_context_get_display at all here, then - I would just pass NULL to notify_desktop_launch. The only consumer of that dbus signal (gnome-session), doesn't look at the display at all...
Review of attachment 311370 [details] [review]: Looks good to me, module formatting niggles ::: gdk/x11/gdkapplaunchcontext-x11.c @@ +470,3 @@ NULL); + display_name = g_app_launch_context_get_display(G_APP_LAUNCH_CONTEXT(ctx), Space before ( here, please (twice) @@ +473,3 @@ + NULL, NULL); + if (display_name) + g_app_launch_context_setenv (G_APP_LAUNCH_CONTEXT(ctx), and here too
Review of attachment 311371 [details] [review]: Lets hold off on this. Wayland is supposed to be better than this.
Created attachment 311835 [details] [review] glib/gio patch (updated)
Created attachment 311836 [details] [review] gtk+/gdk/x11 patch (updated)
Created attachment 311837 [details] [review] gtk+/gdk/wayland patch (updated) Won't be merging this one, just updating formatting for consistency :)
Pushed the gio and x11 patches. Lets hold the Wayland patch - I really want us to move away from environment variables for this, so lets try to get by without DISPLAY.
What is the impact of this on glib-only command line helpers that spawn gtk apps? For instance I use gio networking in a session daemon and expose a method to bring up an UI.
(In reply to Paolo Borelli from comment #16) > What is the impact of this on glib-only command line helpers that spawn gtk > apps? For instance I use gio networking in a session daemon and expose a > method to bring up an UI. Not sure I understand your question, does it fail in your case now with these patches applied?
I have not tested yet, I am just wondering if this will require changes on my part and if that is the case they should be documented (e.g. giving a suggestion on where we should set display ourselves). (For what is worth I agree with the change, I just want to know/document how we need to modify our code)
if DISPLAY was set before in the environment, it's still there so it should not change much. It just won't be changed to a wrong value on Wayland.
That's not what I meant... before DISPLAY was set for me, now I will have to set it myself, no? If yes, where?
(In reply to Paolo Borelli from comment #20) > That's not what I meant... before DISPLAY was set for me, now I will have to > set it myself, no? If yes, where? My point was, it depends how you use the API and from where your application is being run. If it's from within X then DISPLAY would be set and in the environment already - If not, then you may have to set it prior to call the gio API just like gdk does now (with these patches).
DISPLAY was only set for you if you were using a GdkAppLaunchContext that provides a display value. That did not change.