Only send active window changes to X11 root window if the X11 window changed
ClosedPublic

Authored by graesslin on Aug 3 2017, 3:00 PM.

Details

Summary

So far KWin always updated the active window property even if the actual
window id hasn't changed. E.g. if a Wayland window was active and another
Wayland window gets activated the window id was and stays 0.

Nevertheless KWin updated the property causing wakeups in X server and
any application listening to property changes on the root window.

Futhermore this situation is an information leak: we leak when a Wayland
window gets activated to X11.

To solve this problem RootInfo caches the active window id and only
updates if it changes.

Test Plan

Verified with xev -root that the active window does not get
updated needlessly.

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
graesslin created this revision.Aug 3 2017, 3:00 PM
Restricted Application added a project: KWin. · View Herald TranscriptAug 3 2017, 3:00 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
davidedmundson requested changes to this revision.Aug 19 2017, 3:24 PM
davidedmundson added a subscriber: davidedmundson.

under X, any other process can change _net_active_window

If that happens we need our m_activeWindow to reflect that, otherwise kwin can be out of sync and not send anything.

NetRootInfo already contains an activeWindow() method, which seems up-to-date, can we use that instead of the member var?

This revision now requires changes to proceed.Aug 19 2017, 3:24 PM

under X, any other process can change _net_active_window

No! Other processes are not allowed to change _net_active_window, see https://specifications.freedesktop.org/wm-spec/wm-spec-latest.html#idm140200472702304

Important quote: "This is a read-only property set by the Window Manager."

If that happens we need our m_activeWindow to reflect that, otherwise kwin can be out of sync and not send anything.

KWin does not detect if a client changes the property directly. Didn't before. If that ever happened it went out of sync. Similar for any other Window Manager property (c.f. events.cpp, search for rootInfo()->event). We don't read them back. I do not see any reason to adjust this.

NetRootInfo already contains an activeWindow() method, which seems up-to-date, can we use that instead of the member var?

No, we cannot. The setActiveWindow method does not store the value. So once you call setActiveWindow, activeWindow has a wrong value till one received the property change and passes it to the NetRootInfo. So no, it's not up-to-date.

David has some point though - m_activeWindow *can* get out of sync (server error, mal... stupid client - and will be temporarily due to the async setup) and must not be used directly to query the active window.

Since there should be only one root per process, maybe rather use a local static than a member (to constrain this cache to the particular function)?

David has some point though - m_activeWindow *can* get out of sync (server error, mal... stupid client - and will be temporarily due to the async setup) and must not be used directly to query the active window.

Yes David has a point. But this commit doesn't change anything. KWin never tried to restore the active client in case of server error or stupid client. When the next client gets activated it would be set - the same as with this change. If we want to protect against this, that's fine with me, but it's orthogonal to this change.

Since there should be only one root per process, maybe rather use a local static than a member (to constrain this cache to the particular function)?

I fail to see what should be better with a local static than a member variable (also don't take the one root per process for granted, if KWin/Wayland at some distant future point handles a crashing XWayland gracefully it would no longer be granted).

There's no technical advantage in a local static, only a design one. You prevent it from future mis-use beyond its limited purpose, because m_activeWindow isn't actually a "property" of RootInfo.
If you however at some point need several instances, that's no longer an option (resp. you'd need a local hash which is but even uglier than the "false" member)

Likewise this is not about protecting* the active window, but if a client does shit, RootInfo::activeWindow() still reflects a common idea, while m_activeWindow does not (which sets up the case for the above arguement to protect m_activeWindow against unqualified reads to query the active window - which i think is at least part of Davids concern)

*questionale itfp, it's rather race-prone and there's just a bug in the other client that needs to be fixed.

So am I wrong in thinking on X, our favourite method KWindowSystem::forceActiveWindow changes _net_active_window on the root window?

My concern is:

  • Typing in client A
  • Client B does KWindowSystem::forceWindowActive()
  • User ignores it and selects client A again
  • Before this patch A gets definitely activated
  • After this patch, does it?

Unless things changed for the worse, clients (through this function) still send a client message (on _NET_ACTIVE_WINDOW) - the difference between the regular call and the force version is the source indication being "2" ("i'm a pager/taskbar and know what i'm doing so please do") - but the actual property is still set by the WM after receiving (and honoring) such message.

IOW, whenever the property on the root window is set by anything but kwin, anything™ has a bug. Period.

This member variable will systematically get out of in a race between kwin and the X11 server (ie. you can be sure there're some ns - ms where m_activeWindow will differ from ::activeWindow()) but any other case is clearly misbehavior in a 3rd process (or the X11 server failing to handle the property update request)

davidedmundson accepted this revision.Aug 20 2017, 2:27 PM

@luebking right, thanks for clarifying that.

This revision is now accepted and ready to land.Aug 20 2017, 2:27 PM
This revision was automatically updated to reflect the committed changes.