X11 windows cannot be shared in browsers (Firefox, Chromium)

[slash3g]

X11 Windows created using SDL_CreateWindow cannot be shared in browser (see also Genymobile/scrcpy#2640).
The bug has been introduced in b3b4677.
Apparently, both _NET_WM_NAME and WM_NAME need to be set in order for the browser to find the window.

The following (quick and dirty) patch will fix this behaviour:

diff --git a/src/video/x11/SDL_x11window.c b/src/video/x11/SDL_x11window.c
index 8d81cb055..f083d782b 100644
--- a/src/video/x11/SDL_x11window.c
+++ b/src/video/x11/SDL_x11window.c
@@ -735,6 +735,9 @@ X11_SetWindowTitle(_THIS, SDL_Window * window)
     Atom UTF8_STRING = data->videodata->UTF8_STRING;
     Atom _NET_WM_NAME = data->videodata->_NET_WM_NAME;
 
+    Atom WM_NAME = X11_XInternAtom(data->videodata->display, "WM_NAME", False);
+    X11_XChangeProperty(display, data->xwindow, WM_NAME, UTF8_STRING, 8, 0, (const unsigned char *) title, strlen(title));
+
     status = X11_XChangeProperty(display, data->xwindow, _NET_WM_NAME, UTF8_STRING, 8, 0, (const unsigned char *) title, strlen(title));
 
     if (status != 1) {

Moreover, using xdotool to rename (and set both properties of) a window can be used to work around this issue:

xdotool selectwindow set_window --name "WindowName"

[slouken]

Fixed, thanks!

[morgant]

Thanks, @slouken! This should fix an issue I'm seeing with @openbsd's fvwm not being able to match against SDL window names because they're lacking a WM_TITLE.

That said, does your fix in 6c56e27 reintroduce old issues (at least in older window managers that don't support _NET_WM_NAME)? I believe, at least according to Issue #35, that WM_NAME needs to be a TEXT String, not a UTF-8 String, so should be converted before being set. Such conversion was removed by @ctrlcctrlv's PR #4290.

[slouken]

That said, does your fix in 6c56e27 reintroduce old issues (at least in older window managers that don't support _NET_WM_NAME)? I believe, at least according to Issue #35, that WM_NAME needs to be a TEXT String, not a UTF-8 String, so should be converted before being set. Such conversion was removed by @ctrlcctrlv's PR #4290.

Could you provide a PR with a tested patch? I don't have a test environment for this right now.

[ctrlcctrlv]

Yes, I removed WM_NAME because of its encoding issues, if you'd like to reintroduce it you're going to need to use function X11_XSetTextProperty as it was before c5dd996

[ctrlcctrlv]

Also make sure you test patches against the test case in #4288 which prompted #4290.

[slouken]

Is this something you guys can fix for 2.0.18? I'd like to ship without this regression, if possible.

[morgant]

@slouken I don't have much availability to work on this except for this coming weekend (2021-11-27 & 28). I see 2.0.18 is already a few days overdue, so I'm not sure if that's soon enough or if there's enough else pending that it'd be okay.

[slouken]

There's enough pending that this weekend would be fine.

Thanks!

[sezero]

What happened to this one? Still meant for 2.0.18?

[slouken]

Yes, morgant is going to get this tested and in for 2.0.18.

[slouken]

@morgant, this is the last item going into 2.0.18. Do you have something ready for tomorrow?

[ctrlcctrlv]

@slouken I'm not a regular SDL contributor and the patch under discussion is the only one I've ever written. I have available time going for me, but maybe my lack of experience might make you weary of having me try to fix it. Also, you might be able to rightly say I shouldn't fix the mess I made, although I'd say that reliance on WM_NAME is a Chrome/FF bug. Nevertheless, I'm willing to help if @morgant can't.

[ctrlcctrlv]

I'm looking at the code a little harder and I actually think that the "quick and dirty" patch is really close to being the right one. The previous code only existed to try to run Xorg under a non-Unicode locale. I would say that running Xorg that way is extremely rare these days, probably even non-existent, and likely to cause other issues, but I think I can write a patch pretty quickly here given we know 99% of the problem, I just need to verify some things about Xorg, locales, and whether we should be using XSetWMName.

[slouken]

Sure, go for it. Can you coordinate with @morgant?
Ryan and I will review it, so you’ve got us as safety net as well.

[ctrlcctrlv]

Will do, working on it now.

[morgant]

@slouken & @ctrlcctrlv, my apologies for missing your replies yesterday. I have been working on a patch (morgant@b9b5c04). I'm still testing as I'm also not a regular SDL contributor.

As for the reliance on WM_NAME, it's part of the spec that the client SHOULD set the _NET_WM_NAME and the WM should use this in preference to WM_NAME, but not all WMs support _NET_WM_NAME (yes, even in this day and age, see #4979). Additionally, WM_NAME should be a non-UTF-8 string where _NET_WM_NAME should always be a UTF-8 string, as far as I understand. So, I don't see this as a bug in Chrome & FF, and believe we should be setting both WM_NAME & _NET_WM_NAME.

With the aforementioned string encodings in mind, I believe the cause of issue #4288 was that X11_SetWindowTitle() was using SDL_iconv_utf8_locale() to convert to the UTF-8 title string to the user's locale, but in many (most?) cases is probably still UTF-8.

Also, I believe this same fix needs to be applied to X11_MessageBoxCreateWindow() in https://github.com/morgant/SDL/commit/b9b5c04c56fc14382dfb8be6c58a395fd8f41234.

[ctrlcctrlv]

I'm sorry, I have some thoughts on your patch based on my research.

Firstly, it is wrong to say that WM_NAME should be ASCII. Actually, it should be in the server's locale, and Xorg supplies functions for this. Please see proof at https://tronche.com/gui/x/icccm/sec-2.html#s-2.7.1 , which mentions that the modern encoding for it is COMPOUND_TEXT.

Xutf8TextPropertyToTextList is the most commonly used today to get a COMPOUND_TEXT. It is worth noting that the standard for UTF8_STRING in Xorg was approved of 21 years ago: https://www.irif.fr/~jch/software/UTF8_STRING/UTF8_STRING.text . We should be treating non-existence of X_HAVE_UTF8_STRING as a hard error, or at least a warning.

SDL_iconv_string is wrong because X11 servers can be remote, so we need to ask X itself for the locale. It's easiest to use Xutf8TextPropertyToTextList, because that'll give us back XLocaleNotSupported when and where appropriate. I am concerned that your patch might break things by assuming ASCII where we used to assume UTF-8, as ASCII is true for less people.

Also, I think you should consider using the convenience function XSetWMName to simplify the code.

[ctrlcctrlv]

Also, research showed we're/I'm not the first to assume WM_NAME can finally be done away with, apparently the Qt project assumed the same in 2014 😆 https://herbstluftwm.org/archive/msg00459.html

[morgant]

@ctrlcctrlv Thanks for the feedback!

Yes, you're correct that I should be using a TextProperty so that it's correctly encoded as STRING, COMPOUND_TEXT (an implementation of ISO-2022, as I understand it), or C_STRING (for ASCII), based on the locale instead of forcing it to ASCII. I'm updating my implementation & testing now (sorry, my dev machine is a bit slow).

As for assuming WM_NAME can finally be done away with, even Qt seems to have reversed their stance due to issues in the wild (mainly for MWs that only support the ICCCM features and not EWMH [Extended Window Manager Hints], but also other applications functionality like we're running into w/Chrome/FF).

I do agree that the lack of X_HAVE_UTF8_STRING should probably at least be a warning, at this point. There's still a fair amount of use of it throughout the SDL X11 code, so I wanted to be consistent with existing implementation.

[ctrlcctrlv]

Heh, here I am removing it everywhere, maybe I should stop.

I rewound an old repository to 2010 to make absolute sure I was using XmbTextListToTextProperty correctly: exg/rxvt-unicode@d7d9875 just because I saw the CHANGELOG in Google 😆

I'm sorry I messed up the release cycle of such an important project so I'm doing my best to make sure whatever comes next will be as close to perfect as is possible

[ctrlcctrlv]

Although in retrospect after writing that I think that a lot of the problems in this codebase are from people trying to maintain consistency with the existing implementation instead of changing what needs to be changed. I just saw X_HAVE_UTF8_STRING and SDL_X11_HAVE_UTF8_STRING used in the same function, seeming to be no rhyme or reason to the choice 💫, so maybe I won't stop, even if my change is too large to be considered this go round

[morgant]

Ah, I see you beat me to pretty much the same solution while I was futzing with testing, though you also addressed X11_MessageBoxCreateWindow().

[ctrlcctrlv]

I believe I did all the testing I can think of (ran my original example inside LANG=C Xephyr, under XWayland, under kwin_x11), but I am sure more is possible. Given how my first patch went, I'd encourage you to throw anything title related you can think of at this one :)

[morgant]

I'd be curious what xprop shows for WM_NAME in the test code from #4288 (whether it's now no longer clipped), but I've unfortunately run out of time to test until Tuesday. Many thanks for helping get this resolved!