nxagent: Pass down if window manager has been detected

[uli42]

At start a rootless nxagent checks if the real X server has a Window Manager running. It uses a standard detection routine that tries to select a special input (SubStructureRedirect). As only one client per X server is allowed to select that input one can deduce from the success of this operation if a Window Manager is running.

If nxagent is run in rootless mode and has not found a Window Manager on the real X server it will grab all input (see Screen.c:nxagentOpenScreen).

If any client of the nxagent runs the standard Window Manager detection routine against a rootless nxagent it will not see the Window Manager. If this client happens to be a rootless nxagent again it will then grab all input which is undesired here. Other clients might do other undesired stuff in that case.

To avoid all that a rootless nxagent now tries to detect if a client runs that detection routine and returns the result of its own check to the client.

[uli42]

It is called from the dispatcher (dix/dispatch.c or NXdispatch) once a client calls InputSelect.

Would you like to see a backtrace?

Mike Gabriel [email protected] schrieb am Sa., 10. Aug. 2019, 13:55:

@sunweaver requested changes on this pull request.

I don't like this approach of hacking nxagent code paths into the the DIX. See my inline comments.

Please get back to me, if you have any questions about my reasoning.

Thanks, Mike

In nx-X11/programs/Xserver/dix/events.c https://github.com/ArcticaProject/nx-libs/pull/830#discussion_r312700499 :

EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask) +#endif

I don't like this one. EventSelectForWindow is called from within ChangeWindowAttributes (in dix/window.c). ChangeWindowAttribute is a method set in the pScreen struct. We do define pScreen->ChangeWindowAttributes in Screen.c and assign nxagentChangeWindowAttributes (Window.c) to it. In nxagentChangeWindowAttributes we call XChangeWindowAttributes after doing some nxagent magic.

So, on what code path is the above EventSelectForWindow function really called insider nxagent? I feel that we should handle this a bit more object oriented, that is: define an nxagentEventSelectForWindow and have that called from whereever. Also, I'd prefer to see dix/events.c patched in a way, that ChangeWindowAttributes calls nxagentEventSelectForWindow if NXAGENT_SERVER is defined, EventSelectForWindow (from X.Org), if not defined.

In nx-X11/programs/Xserver/hw/nxagent/NXevents.c https://github.com/ArcticaProject/nx-libs/pull/830#discussion_r312700503 :

@@ -520,6 +520,37 @@ DefineInitialRootWindow(register WindowPtr win) } }

+#ifdef NXAGENT_SERVER +extern Bool nxagentWMIsRunning; + +int +EventSelectForWindow(register WindowPtr pWin, register ClientPtr client, Mask mask)

please rename to nxagentEventSelectForWindow and let it be called from whereever needed.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ArcticaProject/nx-libs/pull/830?email_source=notifications&email_token=ABQHBZHG42C53DGPSXBY5GDQD2UE3A5CNFSM4IKSJTVKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBGBVCY#pullrequestreview-273422987, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQHBZBNOEFW6STC7DTQ4F3QD2UE3ANCNFSM4IKSJTVA .

[uli42]

Here's the according backtrace:

(gdb) bt #0 EventSelectForWindow (pWin=pWin@entry=0x555555b71bb0, client=client@entry=0x555555ca1e20, mask=1048576) at NXevents.c:529 #1 0x000055555559d7c0 in ChangeWindowAttributes (pWin=pWin@entry=0x555555b71bb0, vmask=2048, vlist=vlist@entry=0x555555ca1ffc, client=client@entry=0x555555ca1e20) at ../../dix/window.c:1291 #2 0x00005555555b3822 in ProcChangeWindowAttributes (client=0x555555ca1e20) at ../../dix/dispatch.c:525 #3 0x00005555555bae98 in Dispatch () at NXdispatch.c:476 #4 0x0000555555599cb1 in main (argc=4, argv=0x7fffffffdd18, envp=) at main.c:353

Note that nxagentChangeWindowAttributes is not called at all! dix's ChangeWindowAttributes calls nxagentChangeWindowAttributes at the end, but EventSelectForWindow is called before.

This is all internal functionality so we need to intercept that somehow. It is just the same as most other call interceptions we do.

Uli

Update: pasted the correct backtrace...

[uli42]

After having another look: while we could check all this nxagentChangeWindowAttributes we could not return a result because ChangeWindowAttributes calls it without any rc check. So we have these alternatives:

1. patch dix/window.c:ChangeWindowAttributes (we don't want that)
2. replace ChangeWindowAttributes by an own function
3. patch EventSelectForWindow (that's what I did). It is only called from ChangeWindowAttributes.

[sunweaver]

Hi,

On Sa 10 Aug 2019 15:18:20 CEST, Ulrich Sibiller wrote:

After having another look: while we could check all this
nxagentChangeWindowAttributes we could not return a result because
ChangeWindowAttributes calls it without any rc check. So we have
these alternatives:

Ok...

1. patch dix/window.c:ChangeWindowAttributes (we don't want that)

Why don't we want that?

#ifdef NXAGENT_SERVER
    ... nxagentEventSelectForWindow(...)
#else
    ... EventSelectForWindow(...)
#endif

license wise, this is unproblematic. It only means that we need to get
this ifdef into X.Org upstream one day.

2. replace ChangeWindowAttributes by an own function

This is too much code to have replaced.

3. patch EventSelectForWindow (that's what I did). It is only called
   from ChangeWindowAttributes.

You replace the whole function, I'd prefer seeing the function calls
to EventSelectForWindow conditionally replaced, as this change on
X.Org is much more likely to find acceptance on the X.Org upstream
side, I presume (only guessing here, though).

Mike

--

DAS-NETZWERKTEAM c\o Technik- und Ökologiezentrum Eckernförde Mike Gabriel, Marienthaler str. 17, 24340 Eckernförde mobile: +49 (1520) 1976 148 landline: +49 (4351) 850 8940

GnuPG Fingerprint: 9BFB AEE8 6C0A A5FF BF22 0782 9AF4 6B30 2577 1B31 mail: [email protected], http://das-netzwerkteam.de

[uli42]

On Sat, Aug 10, 2019 at 4:01 PM Mike Gabriel [email protected] wrote:

Hi,

On Sa 10 Aug 2019 15:18:20 CEST, Ulrich Sibiller wrote:

After having another look: while we could check all this nxagentChangeWindowAttributes we could not return a result because ChangeWindowAttributes calls it without any rc check. So we have these alternatives:

Ok...

1. patch dix/window.c:ChangeWindowAttributes (we don't want that)

Why don't we want that?

#ifdef NXAGENT_SERVER
... nxagentEventSelectForWindow(...)
#else
... EventSelectForWindow(...)
#endif

I don''t want do patch in code fragments in the middle of the code. That is just a PITA if you try to apply backports. For me (only) the following are acceptable approaches.

1. replace the whole function via #ifndef NXAGENT + identically named function elsewhere. This the initial approach you took for all the NX*.c files. Downside: we have code duplication which leads to two places where possible backports need to be applied (and adapted to the changed code!)
2. replace the whole function via Xorg_ rename and adding another function named as the original on, possibly calling the renamed Xorg_ variant at some point. This is the approach I favour now as it helps in reducing code duplication. Also we can update dix code and automatically benefit from upstream changes without touching our own code. Further, and most important, it greatly reduces the comparison work that's required when backporting upstream Xorg code.
3. Use an official callback. For this approach we could suggest some more callback hooks to Xorg upstream. But I doubt we will be able to find appropriate callback solutions for all the places where we patch upstream code.

license wise, this is unproblematic. It only means that we need to get this ifdef into X.Org upstream one day.

2. replace ChangeWindowAttributes by an own function

This is too much code to have replaced.

exactly

3. patch EventSelectForWindow (that's what I did). It is only called from ChangeWindowAttributes.

You replace the whole function, I'd prefer seeing the function calls to EventSelectForWindow conditionally replaced, as this change on X.Org is much more likely to find acceptance on the X.Org upstream side, I presume (only guessing here, though).

Yes, and that function is only called from one place in Xorg. And even if it there were more places you'd still want to have a consistent behaviour so all calls should use the replacement function. It is done that way on all places where I introduced the Xorg_ naming scheme.

Uli

[uli42]

You have not responded to my explanations. So what are we going to do with this PR?

[sunweaver]

2. replace the whole function via Xorg_ rename and adding another function named as the original on, possibly calling the renamed Xorg_ variant at some point. This is the approach I favour now as it helps in reducing code duplication. Also we can update dix code and automatically benefit from upstream changes without touching our own code. Further, and most important, it greatly reduces the comparison work that's required when backporting upstream Xorg code.

I prefer variant 2., then...

[uli42]

Currently I do not recommend to merge this. It turned out that java applications in a rootless session struggle with their window manager detection and will then fail to resize properly (window gets bigger, but not content). A workaround is to run them with _JAVA_AWT_WM_NONREPARENTING=1

I think the trick that would fix it was to propagate the root window properties _NET_SUPPORTING_WM_CHECK and _NET_WM_NAME, too. Or set them explicitly like wmname does (https://git.suckless.org/wmname/file/wmname.c.html).

Update: just verifified: using wmname to set the WM to "LG3D" (wile the local side isr running KWin) makes java apps work again. But I am unsure if we can set that regardless of the local window manager.

[sunweaver]

Currently I do not recommend to merge this. It turned out that java applications in a rootless session struggle with their window manager detection and will then fail to resize properly (window gets bigger, but not content). A workaround is to run them with _JAVA_AWT_WM_NONREPARENTING=1

I think the trick that would fix it was to propagate the root window properties _NET_SUPPORTING_WM_CHECK and _NET_WM_NAME, too. Or set them explicitly like wmname does (https://git.suckless.org/wmname/file/wmname.c.html).

Update: just verifified: using wmname to set the WM to "LG3D" (wile the local side isr running KWin) makes java apps work again. But I am unsure if we can set that regardless of the local window manager.

ok. Thanks for the heads up.